Make tests pass again

* test enrolls in course before testing pages
* support github edit links with new file structure
* Some pep8 cleanups

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:

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"

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,

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
-    
+

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

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 = {