diff --git a/common/djangoapps/xmodule_modifiers.py b/common/djangoapps/xmodule_modifiers.py index 597d74ce6f..380388b545 100644 --- a/common/djangoapps/xmodule_modifiers.py +++ b/common/djangoapps/xmodule_modifiers.py @@ -90,10 +90,12 @@ def add_histogram(get_html, module): # TODO (ichuang): Remove after fall 2012 LMS migration done if settings.MITX_FEATURES.get('ENABLE_LMS_MIGRATION'): - [filepath, filename] = module.definition.get('filename','') + [filepath, filename] = module.definition.get('filename', ['', None]) osfs = module.system.filestore if filename is not None and osfs.exists(filename): - filepath = filename # if original, unmangled filename exists then use it (github doesn't like symlinks) + # if original, unmangled filename exists then use it (github + # doesn't like symlinks) + filepath = filename data_dir = osfs.root_path.rsplit('/')[-1] edit_link = "https://github.com/MITx/%s/tree/master/%s" % (data_dir,filepath) else: diff --git a/common/lib/xmodule/xmodule/tests/test_export.py b/common/lib/xmodule/xmodule/tests/test_export.py index 9d2f33eaee..bcbf81861c 100644 --- a/common/lib/xmodule/xmodule/tests/test_export.py +++ b/common/lib/xmodule/xmodule/tests/test_export.py @@ -26,12 +26,14 @@ def strip_metadata(descriptor, key): for d in descriptor.get_children(): strip_metadata(d, key) -def check_gone(descriptor, key): - '''Make sure that the metadata of this descriptor or any - descendants does not include key''' - assert_true(key not in descriptor.metadata) +def strip_filenames(descriptor): + """ + Recursively strips 'filename' from all children's definitions. + """ + print "strip filename from {desc}".format(desc=descriptor.location.url()) + descriptor.definition.pop('filename', None) for d in descriptor.get_children(): - check_gone(d, key) + strip_filenames(d) @@ -70,6 +72,11 @@ class RoundTripTestCase(unittest.TestCase): strip_metadata(initial_course, 'data_dir') strip_metadata(exported_course, 'data_dir') + # HACK: filenames change when changing file formats + # during imports from old-style courses. Ignore them. + strip_filenames(initial_course) + strip_filenames(exported_course) + self.assertEquals(initial_course, exported_course) print "Checking key equality" diff --git a/common/lib/xmodule/xmodule/xml_module.py b/common/lib/xmodule/xmodule/xml_module.py index c96728ffba..c9fee6c9a5 100644 --- a/common/lib/xmodule/xmodule/xml_module.py +++ b/common/lib/xmodule/xmodule/xml_module.py @@ -70,7 +70,7 @@ class XmlDescriptor(XModuleDescriptor): # Related: What's the right behavior for clean_metadata? metadata_attributes = ('format', 'graceperiod', 'showanswer', 'rerandomize', 'start', 'due', 'graded', 'display_name', 'url_name', 'hide_from_toc', - 'ispublic', # if True, then course is listed for all users; see + 'ispublic', # if True, then course is listed for all users; see # VS[compat] Remove once unused. 'name', 'slug') @@ -171,8 +171,7 @@ class XmlDescriptor(XModuleDescriptor): # again in the correct format. This should go away once the CMS is # online and has imported all current (fall 2012) courses from xml if not system.resources_fs.exists(filepath) and hasattr( - cls, - 'backcompat_paths'): + cls, 'backcompat_paths'): candidates = cls.backcompat_paths(filepath) for candidate in candidates: if system.resources_fs.exists(candidate): @@ -233,13 +232,19 @@ class XmlDescriptor(XModuleDescriptor): # VS[compat] -- detect new-style each-in-a-file mode if is_pointer_tag(xml_object): # new style: - # read the actual defition file--named using url_name + # read the actual definition file--named using url_name filepath = cls._format_filepath(xml_object.tag, url_name) definition_xml = cls.load_file(filepath, system.resources_fs, location) else: definition_xml = xml_object definition = cls.load_definition(definition_xml, system, location) + # VS[compat] -- make Ike's github preview links work in both old and + # new file layouts + if is_pointer_tag(xml_object): + # new style -- contents actually at filepath + definition['filename'] = [filepath, filepath] + metadata = cls.load_metadata(definition_xml) return cls( system, diff --git a/lms/djangoapps/courseware/courses.py b/lms/djangoapps/courseware/courses.py index 8193988d67..47d5777f8d 100644 --- a/lms/djangoapps/courseware/courses.py +++ b/lms/djangoapps/courseware/courses.py @@ -46,12 +46,15 @@ def check_course(course_id, course_must_be_open=True, course_required=True): def course_image_url(course): - return staticfiles_storage.url(course.metadata['data_dir'] + "/images/course_image.jpg") + return staticfiles_storage.url(course.metadata['data_dir'] + + "/images/course_image.jpg") def get_course_about_section(course, section_key): """ - This returns the snippet of html to be rendered on the course about page, given the key for the section. + This returns the snippet of html to be rendered on the course about page, + given the key for the section. + Valid keys: - overview - title @@ -70,18 +73,23 @@ def get_course_about_section(course, section_key): - more_info """ - # Many of these are stored as html files instead of some semantic markup. This can change without effecting - # this interface when we find a good format for defining so many snippets of text/html. + # Many of these are stored as html files instead of some semantic + # markup. This can change without effecting this interface when we find a + # good format for defining so many snippets of text/html. # TODO: Remove number, instructors from this list - if section_key in ['short_description', 'description', 'key_dates', 'video', 'course_staff_short', 'course_staff_extended', - 'requirements', 'syllabus', 'textbook', 'faq', 'more_info', 'number', 'instructors', 'overview', - 'effort', 'end_date', 'prerequisites']: + if section_key in ['short_description', 'description', 'key_dates', 'video', + 'course_staff_short', 'course_staff_extended', + 'requirements', 'syllabus', 'textbook', 'faq', 'more_info', + 'number', 'instructors', 'overview', + 'effort', 'end_date', 'prerequisites']: try: with course.system.resources_fs.open(path("about") / section_key + ".html") as htmlFile: - return replace_urls(htmlFile.read().decode('utf-8'), course.metadata['data_dir']) + return replace_urls(htmlFile.read().decode('utf-8'), + course.metadata['data_dir']) except ResourceNotFoundError: - log.warning("Missing about section {key} in course {url}".format(key=section_key, url=course.location.url())) + log.warning("Missing about section {key} in course {url}".format( + key=section_key, url=course.location.url())) return None elif section_key == "title": return course.metadata.get('display_name', course.url_name) @@ -95,7 +103,9 @@ def get_course_about_section(course, section_key): def get_course_info_section(course, section_key): """ - This returns the snippet of html to be rendered on the course info page, given the key for the section. + This returns the snippet of html to be rendered on the course info page, + given the key for the section. + Valid keys: - handouts - guest_handouts @@ -103,43 +113,51 @@ def get_course_info_section(course, section_key): - guest_updates """ - # Many of these are stored as html files instead of some semantic markup. This can change without effecting - # this interface when we find a good format for defining so many snippets of text/html. + # Many of these are stored as html files instead of some semantic + # markup. This can change without effecting this interface when we find a + # good format for defining so many snippets of text/html. if section_key in ['handouts', 'guest_handouts', 'updates', 'guest_updates']: try: with course.system.resources_fs.open(path("info") / section_key + ".html") as htmlFile: - return replace_urls(htmlFile.read().decode('utf-8'), course.metadata['data_dir']) + return replace_urls(htmlFile.read().decode('utf-8'), + course.metadata['data_dir']) except ResourceNotFoundError: - log.exception("Missing info section {key} in course {url}".format(key=section_key, url=course.location.url())) + log.exception("Missing info section {key} in course {url}".format( + key=section_key, url=course.location.url())) return "! Info section missing !" raise KeyError("Invalid about key " + str(section_key)) def course_staff_group_name(course): ''' - course should be either a CourseDescriptor instance, or a string (the .course entry of a Location) + course should be either a CourseDescriptor instance, or a string (the + .course entry of a Location) ''' - if isinstance(course,str): + if isinstance(course, str) or isinstance(course, unicode): coursename = course else: - coursename = course.metadata.get('data_dir','UnknownCourseName') - if not coursename: # Fall 2012: not all course.xml have metadata correct yet - coursename = course.metadata.get('course','') + # should be a CourseDescriptor, so grab its location.course: + coursename = course.location.course return 'staff_%s' % coursename -def has_staff_access_to_course(user,course): +def has_staff_access_to_course(user, course): ''' Returns True if the given user has staff access to the course. This means that user is in the staff_* group, or is an overall admin. + + course is the course field of the location being accessed. ''' if user is None or (not user.is_authenticated()) or course is None: return False if user.is_staff: return True - user_groups = [x[1] for x in user.groups.values_list()] # note this is the Auth group, not UserTestGroup + + # note this is the Auth group, not UserTestGroup + user_groups = [x[1] for x in user.groups.values_list()] staff_group = course_staff_group_name(course) - log.debug('course %s user %s groups %s' % (staff_group, user, user_groups)) + log.debug('course %s, staff_group %s, user %s, groups %s' % ( + course, staff_group, user, user_groups)) if staff_group in user_groups: return True return False @@ -154,7 +172,8 @@ def get_courses_by_university(user): Returns dict of lists of courses available, keyed by course.org (ie university). Courses are sorted by course.number. - if ACCESS_REQUIRE_STAFF_FOR_COURSE then list only includes those accessible to user. + if ACCESS_REQUIRE_STAFF_FOR_COURSE then list only includes those accessible + to user. ''' # TODO: Clean up how 'error' is done. # filter out any courses that errored. @@ -168,4 +187,4 @@ def get_courses_by_university(user): continue universities[course.org].append(course) return universities - + diff --git a/lms/djangoapps/courseware/tests/tests.py b/lms/djangoapps/courseware/tests/tests.py index 0e2f7d7aa5..e0369baf7b 100644 --- a/lms/djangoapps/courseware/tests/tests.py +++ b/lms/djangoapps/courseware/tests/tests.py @@ -135,10 +135,25 @@ class ActivateLoginTestCase(TestCase): class PageLoader(ActivateLoginTestCase): ''' Base class that adds a function to load all pages in a modulestore ''' + + def enroll(self, course): + resp = self.client.post('/change_enrollment', { + 'enrollment_action': 'enroll', + 'course_id': course.id, + }) + data = parse_json(resp) + self.assertTrue(data['success']) + def check_pages_load(self, course_name, data_dir, modstore): print "Checking course {0} in {1}".format(course_name, data_dir) import_from_xml(modstore, data_dir, [course_name]) + # enroll in the course before trying to access pages + courses = modstore.get_courses() + self.assertEqual(len(courses), 1) + course = courses[0] + self.enroll(course) + n = 0 num_bad = 0 all_ok = True diff --git a/lms/envs/common.py b/lms/envs/common.py index 83a4bd4181..bd77e711b3 100644 --- a/lms/envs/common.py +++ b/lms/envs/common.py @@ -157,6 +157,9 @@ COURSE_SETTINGS = {'6.002x_Fall_2012': {'number' : '6.002x', } } +# IP addresses that are allowed to reload the course, etc. +# TODO (vshnayder): Will probably need to change as we get real access control in. +LMS_MIGRATION_ALLOWED_IPS = [] ############################### XModule Store ################################## MODULESTORE = {