Merge pull request #337 from MITx/feature/victor/use-url-names

Feature/victor/use url names

cms/djangoapps/contentstore/views.py

@@ -108,7 +108,7 @@ def edit_item(request):
         'contents': item.get_html(),
         'js_module': item.js_module_name,
         'category': item.category,
-        'name': item.name,
+        'url_name': item.url_name,
         'previews': get_module_previews(request, item),
     })
 

cms/templates/unit.html

 <section id="unit-wrapper">
   <header>
     <section>
-      <h1 class="editable">${name}</h1>
+      <h1 class="editable">${url_name}</h1>
       <p class="${category}"><a href="#">${category}</a></p>
     </section>
 

cms/templates/widgets/navigation.html

@@ -41,7 +41,7 @@
     % for week in weeks:
     <li class="week" data-id="${week.location.url()}">
       <header>
-        <h1><a href="#" class="week-edit">${week.name}</a></h1>
+        <h1><a href="#" class="week-edit">${week.url_name}</a></h1>
         <ul>
           % if 'goals' in week.metadata:
             % for goal in week.metadata['goals']:
@@ -60,7 +60,7 @@
             data-type="${module.js_module_name}"
             data-preview-type="${module.module_class.js_module_name}">
 
-            <a href="#" class="module-edit">${module.name}</a>
+            <a href="#" class="module-edit">${module.url_name}</a>
             <a href="#" class="draggable">handle</a>
         </li>
         % endfor

cms/templates/widgets/sequence-edit.html

@@ -39,7 +39,7 @@
               <a href="#" class="module-edit"
                  data-id="${child.location.url()}"
                  data-type="${child.js_module_name}"
-                 data-preview-type="${child.module_class.js_module_name}">${child.name}</a>
+                 data-preview-type="${child.module_class.js_module_name}">${child.url_name}</a>
               <a href="#" class="draggable">handle</a>
             </li>
             %endfor

common/lib/capa/capa/capa_problem.py

@@ -288,20 +288,30 @@ class LoncapaProblem(object):
                 try:
                     ifp = self.system.filestore.open(file)  # open using ModuleSystem OSFS filestore
                 except Exception as err:
-                    log.error('Error %s in problem xml include: %s' % (err, etree.tostring(inc, pretty_print=True)))
-                    log.error('Cannot find file %s in %s' % (file, self.system.filestore))
-                    if not self.system.get('DEBUG'):		# if debugging, don't fail - just log error
+                    log.error('Error %s in problem xml include: %s' % (
+                            err, etree.tostring(inc, pretty_print=True)))
+                    log.error('Cannot find file %s in %s' % (
+                            file, self.system.filestore))
+                    # if debugging, don't fail - just log error
+                    # TODO (vshnayder): need real error handling, display to users
+                    if not self.system.get('DEBUG'):
                         raise
-                    else: continue
+                    else:
+                        continue
                 try:
                     incxml = etree.XML(ifp.read())		# read in and convert to XML
                 except Exception as err:
-                    log.error('Error %s in problem xml include: %s' % (err, etree.tostring(inc, pretty_print=True)))
+                    log.error('Error %s in problem xml include: %s' % (
+                            err, etree.tostring(inc, pretty_print=True)))
                     log.error('Cannot parse XML in %s' % (file))
-                    if not self.system.get('DEBUG'):		# if debugging, don't fail - just log error
+                    # if debugging, don't fail - just log error
+                    # TODO (vshnayder): same as above
+                    if not self.system.get('DEBUG'):
                         raise
-                    else: continue
-                parent = inc.getparent()			# insert  new XML into tree in place of inlcude
+                    else:
+                        continue
+                # insert new XML into tree in place of inlcude
+                parent = inc.getparent()
                 parent.insert(parent.index(inc), incxml)
                 parent.remove(inc)
                 log.debug('Included %s into %s' % (file, self.problem_id))

common/lib/xmodule/xmodule/html_module.py

@@ -121,13 +121,13 @@ class HtmlDescriptor(XmlDescriptor, EditingDescriptor):
 
         # Not proper format.  Write html to file, return an empty tag
         filepath = u'{category}/{name}.html'.format(category=self.category,
-                                                    name=self.name)
+                                                    name=self.url_name)
 
         resource_fs.makedir(os.path.dirname(filepath), allow_recreate=True)
         with resource_fs.open(filepath, 'w') as file:
             file.write(self.definition['data'])
 
         elt = etree.Element('html')
-        elt.set("filename", self.name)
+        elt.set("filename", self.url_name)
         return elt
 

common/lib/xmodule/xmodule/modulestore/search.py

@@ -87,6 +87,7 @@ def path_to_location(modulestore, location, course_name=None):
 
     n = len(path)
     course_id = CourseDescriptor.location_to_id(path[0])
+    # pull out the location names
     chapter = path[1].name if n > 1 else None
     section = path[2].name if n > 2 else None
 

common/lib/xmodule/xmodule/video_module.py

@@ -23,11 +23,12 @@ class VideoModule(XModule):
     css = {'scss': [resource_string(__name__, 'css/video/display.scss')]}
     js_module_name = "Video"
 
-    def __init__(self, system, location, definition, instance_state=None, shared_state=None, **kwargs):
-        XModule.__init__(self, system, location, definition, instance_state, shared_state, **kwargs)
+    def __init__(self, system, location, definition,
+                 instance_state=None, shared_state=None, **kwargs):
+        XModule.__init__(self, system, location, definition,
+                         instance_state, shared_state, **kwargs)
         xmltree = etree.fromstring(self.definition['data'])
         self.youtube = xmltree.get('youtube')
-        self.name = xmltree.get('name')
         self.position = 0
 
         if instance_state is not None:
@@ -71,7 +72,7 @@ class VideoModule(XModule):
             'streams': self.video_list(),
             'id': self.location.html_id(),
             'position': self.position,
-            'name': self.name,
+            'display_name': self.display_name,
             # TODO (cpennington): This won't work when we move to data that isn't on the filesystem
             'data_dir': self.metadata['data_dir'],
         })

common/lib/xmodule/xmodule/x_module.py

@@ -191,11 +191,20 @@ class XModule(HTMLSnippet):
         self.instance_state = instance_state
         self.shared_state = shared_state
         self.id = self.location.url()
-        self.name = self.location.name
+        self.url_name = self.location.name
         self.category = self.location.category
         self.metadata = kwargs.get('metadata', {})
         self._loaded_children = None
 
+    @property
+    def display_name(self):
+        '''
+        Return a display name for the module: use display_name if defined in
+        metadata, otherwise convert the url name.
+        '''
+        return self.metadata.get('display_name',
+                                 self.url_name.replace('_', ' '))
+
     def get_children(self):
         '''
         Return module instances for all the children of this module.
@@ -339,6 +348,8 @@ class XModuleDescriptor(Plugin, HTMLSnippet):
                     module
                 display_name: The name to use for displaying this module to the
                     user
+                url_name: The name to use for this module in urls and other places
+                    where a unique name is needed.
                 format: The format of this module ('Homework', 'Lab', etc)
                 graded (bool): Whether this module is should be graded or not
                 start (string): The date for which this module will be available
@@ -353,7 +364,7 @@ class XModuleDescriptor(Plugin, HTMLSnippet):
         self.metadata = kwargs.get('metadata', {})
         self.definition = definition if definition is not None else {}
         self.location = Location(kwargs.get('location'))
-        self.name = self.location.name
+        self.url_name = self.location.name
         self.category = self.location.category
         self.shared_state_key = kwargs.get('shared_state_key')
 
@@ -361,6 +372,15 @@ class XModuleDescriptor(Plugin, HTMLSnippet):
         self._inherited_metadata = set()
 
     @property
+    def display_name(self):
+        '''
+        Return a display name for the module: use display_name if defined in
+        metadata, otherwise convert the url name.
+        '''
+        return self.metadata.get('display_name',
+                                 self.url_name.replace('_', ' '))
+
+    @property
     def own_metadata(self):
         """
         Return the metadata that is not inherited, but was defined on this module.

common/lib/xmodule/xmodule/xml_module.py

@@ -225,7 +225,7 @@ class XmlDescriptor(XModuleDescriptor):
         # Write it to a file if necessary
         if self.split_to_file(xml_object):
             # Put this object in its own file
-            filepath = self.__class__._format_filepath(self.category, self.name)
+            filepath = self.__class__._format_filepath(self.category, self.url_name)
             resource_fs.makedir(os.path.dirname(filepath), allow_recreate=True)
             with resource_fs.open(filepath, 'w') as file:
                 file.write(etree.tostring(xml_object, pretty_print=True))
@@ -238,10 +238,10 @@ class XmlDescriptor(XModuleDescriptor):
             xml_object.tail = ''
 
 
-            xml_object.set('filename', self.name)
+            xml_object.set('filename', self.url_name)
 
         # Add the metadata
-        xml_object.set('url_name', self.name)
+        xml_object.set('url_name', self.url_name)
         for attr in self.metadata_attributes:
             attr_map = self.xml_attribute_map.get(attr, AttrMap(attr))
             metadata_key = attr_map.metadata_key

lms/djangoapps/courseware/courses.py

@@ -83,7 +83,7 @@ def get_course_about_section(course, section_key):
             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.name)
+        return course.metadata.get('display_name', course.url_name)
     elif section_key == "university":
         return course.location.org
     elif section_key == "number":

lms/djangoapps/courseware/grades.py

@@ -12,18 +12,23 @@ _log = logging.getLogger("mitx.courseware")
 
 def grade_sheet(student, course, grader, student_module_cache):
     """
-    This pulls a summary of all problems in the course. It returns a dictionary with two datastructures:
+    This pulls a summary of all problems in the course. It returns a dictionary
+    with two datastructures:
 
-    - courseware_summary is a summary of all sections with problems in the course. It is organized as an array of chapters,
-    each containing an array of sections, each containing an array of scores. This contains information for graded and ungraded
-    problems, and is good for displaying a course summary with due dates, etc.
+    - courseware_summary is a summary of all sections with problems in the
+    course. It is organized as an array of chapters, each containing an array of
+    sections, each containing an array of scores. This contains information for
+    graded and ungraded problems, and is good for displaying a course summary
+    with due dates, etc.
 
-    - grade_summary is the output from the course grader. More information on the format is in the docstring for CourseGrader.
+    - grade_summary is the output from the course grader. More information on
+      the format is in the docstring for CourseGrader.
 
     Arguments:
         student: A User object for the student to grade
         course: An XModule containing the course to grade
-        student_module_cache: A StudentModuleCache initialized with all instance_modules for the student
+        student_module_cache: A StudentModuleCache initialized with all
+             instance_modules for the student
     """
     totaled_scores = {}
     chapters = []
@@ -51,12 +56,16 @@ def grade_sheet(student, course, grader, student_module_cache):
                         correct = total
 
                 if not total > 0:
-                    #We simply cannot grade a problem that is 12/0, because we might need it as a percentage
+                    #We simply cannot grade a problem that is 12/0, because we
+                    #might need it as a percentage
                     graded = False
 
-                scores.append(Score(correct, total, graded, module.metadata.get('display_name')))
+                scores.append(Score(correct, total, graded,
+                                    module.metadata.get('display_name')))
+
+            section_total, graded_total = graders.aggregate_scores(
+                scores, s.metadata.get('display_name'))
 
-            section_total, graded_total = graders.aggregate_scores(scores, s.metadata.get('display_name'))
             #Add the graded total to totaled_scores
             format = s.metadata.get('format', "")
             if format and graded_total.possible > 0:
@@ -65,7 +74,8 @@ def grade_sheet(student, course, grader, student_module_cache):
                 totaled_scores[format] = format_scores
 
             sections.append({
-                'section': s.metadata.get('display_name'),
+                'display_name': s.display_name,
+                'url_name': s.url_name,
                 'scores': scores,
                 'section_total': section_total,
                 'format': format,
@@ -73,8 +83,9 @@ def grade_sheet(student, course, grader, student_module_cache):
                 'graded': graded,
             })
 
-        chapters.append({'course': course.metadata.get('display_name'),
-                         'chapter': c.metadata.get('display_name'),
+        chapters.append({'course': course.display_name,
+                         'display_name': c.display_name,
+                         'url_name': c.url_name,
                          'sections': sections})
 
     grade_summary = grader.grade(totaled_scores)

lms/djangoapps/courseware/management/commands/clean_xml.py

@@ -61,11 +61,7 @@ def import_with_checks(course_dir, verbose=True):
                    course_dirs=course_dirs)
 
     def str_of_err(tpl):
-        (msg, exc_info) = tpl
-        if exc_info is None:
-            return msg
-
-        exc_str = '\n'.join(traceback.format_exception(*exc_info))
+        (msg, exc_str) = tpl
         return '{msg}\n{exc}'.format(msg=msg, exc=exc_str)
 
     courses = modulestore.get_courses()
@@ -83,7 +79,7 @@ def import_with_checks(course_dir, verbose=True):
         print '\n'
         print "=" * 40
         print 'ERRORs during import:'
-        print '\n'.join(map(str_of_err,errors))
+        print '\n'.join(map(str_of_err, errors))
         print "=" * 40
         print '\n'
 

lms/djangoapps/courseware/module_render.py

@@ -35,10 +35,12 @@ def toc_for_course(user, request, course, active_chapter, active_section):
     Create a table of contents from the module store
 
     Return format:
-    [ {'name': name, 'sections': SECTIONS, 'active': bool}, ... ]
+    [ {'display_name': name, 'url_name': url_name,
+       'sections': SECTIONS, 'active': bool}, ... ]
 
     where SECTIONS is a list
-    [ {'name': name, 'format': format, 'due': due, 'active' : bool}, ...]
+    [ {'display_name': name, 'url_name': url_name,
+       'format': format, 'due': due, 'active' : bool}, ...]
 
     active is set for the section and chapter corresponding to the passed
     parameters.  Everything else comes from the xml, or defaults to "".
@@ -54,19 +56,21 @@ def toc_for_course(user, request, course, active_chapter, active_section):
         sections = list()
         for section in chapter.get_display_items():
 
-            active = (chapter.metadata.get('display_name') == active_chapter and
-                      section.metadata.get('display_name') == active_section)
+            active = (chapter.display_name == active_chapter and
+                      section.display_name == active_section)
             hide_from_toc = section.metadata.get('hide_from_toc', 'false').lower() == 'true'
 
             if not hide_from_toc:
-                sections.append({'name': section.metadata.get('display_name'),
+                sections.append({'display_name': section.display_name,
+                                 'url_name': section.url_name,
                                  'format': section.metadata.get('format', ''),
                                  'due': section.metadata.get('due', ''),
                                  'active': active})
 
-        chapters.append({'name': chapter.metadata.get('display_name'),
+        chapters.append({'display_name': chapter.display_name,
+                         'url_name': chapter.url_name,
                          'sections': sections,
-                         'active': chapter.metadata.get('display_name') == active_chapter})
+                         'active': chapter.display_name == active_chapter})
     return chapters
 
 
@@ -76,8 +80,8 @@ def get_section(course_module, chapter, section):
     or None if this doesn't specify a valid section
 
     course: Course url
-    chapter: Chapter name
-    section: Section name
+    chapter: Chapter url_name
+    section: Section url_name
     """
 
     if course_module is None:
@@ -85,7 +89,7 @@ def get_section(course_module, chapter, section):
 
     chapter_module = None
     for _chapter in course_module.get_children():
-        if _chapter.metadata.get('display_name') == chapter:
+        if _chapter.url_name == chapter:
             chapter_module = _chapter
             break
 
@@ -94,7 +98,7 @@ def get_section(course_module, chapter, section):
 
     section_module = None
     for _section in chapter_module.get_children():
-        if _section.metadata.get('display_name') == section:
+        if _section.url_name == section:
             section_module = _section
             break
 

lms/djangoapps/courseware/views.py

@@ -54,11 +54,6 @@ def user_groups(user):
     return group_names
 
 
-def format_url_params(params):
-    return [urllib.quote(string.replace(' ', '_'))
-            if string is not None else None
-            for string in params]
-
 
 @ensure_csrf_cookie
 @cache_if_anonymous
@@ -124,7 +119,6 @@ def profile(request, course_id, student_id=None):
                'language': user_info.language,
                'email': student.email,
                'course': course,
-               'format_url_params': format_url_params,
                'csrf': csrf(request)['csrf_token']
                }
     context.update(grades.grade_sheet(student, course_module, course.grader, student_module_cache))
@@ -138,9 +132,9 @@ def render_accordion(request, course, chapter, section):
 
         If chapter and section are '' or None, renders a default accordion.
 
-        Returns (initialization_javascript, content)'''
+        Returns the html string'''
 
-    # TODO (cpennington): do the right thing with courses
+    # grab the table of contents
     toc = toc_for_course(request.user, request, course, chapter, section)
 
     active_chapter = 1
@@ -152,7 +146,6 @@ def render_accordion(request, course, chapter, section):
                     ('toc', toc),
                     ('course_name', course.title),
                     ('course_id', course.id),
-                    ('format_url_params', format_url_params),
                     ('csrf', csrf(request)['csrf_token'])] + template_imports.items())
     return render_to_string('accordion.html', context)
 
@@ -169,9 +162,9 @@ def index(request, course_id, chapter=None, section=None,
     Arguments:
 
      - request    : HTTP request
-     - course     : coursename (str)
-     - chapter    : chapter name (str)
-     - section    : section name (str)
+     - course_id  : course id (str: ORG/course/URL_NAME)
+     - chapter    : chapter url_name (str)
+     - section    : section url_name (str)
      - position   : position in module, eg of <sequential> module (str)
 
     Returns:
@@ -180,16 +173,6 @@ def index(request, course_id, chapter=None, section=None,
     '''
     course = check_course(course_id)
 
-    def clean(s):
-        ''' Fixes URLs -- we convert spaces to _ in URLs to prevent
-        funny encoding characters and keep the URLs readable.  This undoes
-        that transformation.
-        '''
-        return s.replace('_', ' ') if s is not None else None
-
-    chapter = clean(chapter)
-    section = clean(section)
-
     try:
         context = {
             'csrf': csrf(request)['csrf_token'],
@@ -202,8 +185,6 @@ def index(request, course_id, chapter=None, section=None,
 
         look_for_module = chapter is not None and section is not None
         if look_for_module:
-            # TODO (cpennington): Pass the right course in here
-
             section_descriptor = get_section(course, chapter, section)
             if section_descriptor is not None:
                 student_module_cache = StudentModuleCache(request.user,

lms/templates/accordion.html

 <%! from django.core.urlresolvers import reverse %>
 
 <%def name="make_chapter(chapter)">
-  <h3><a href="#">${chapter['name']}</a></h3>
+  <h3><a href="#">${chapter['display_name']}</a></h3>
 
   <ul>
     % for section in chapter['sections']:
         <li${' class="active"' if 'active' in section and section['active'] else ''}>
-          <a href="${reverse('courseware_section', args=[course_id] + format_url_params([chapter['name'], section['name']]))}">
-            <p>${section['name']}
+          <a href="${reverse('courseware_section', args=[course_id, chapter['url_name'], section['url_name']])}">
+            <p>${section['display_name']}
             <span class="subtitle">
               ${section['format']} ${"due " + section['due'] if 'due' in section and section['due'] != '' else ''}
             </span>

lms/templates/profile.html

@@ -125,9 +125,9 @@ $(function() {
 
       <ol class="chapters">
         %for chapter in courseware_summary:
-        %if not chapter['chapter'] == "hidden":
+        %if not chapter['display_name'] == "hidden":
         <li>
-          <h2>${ chapter['chapter'] }</h2>
+          <h2>${ chapter['display_name'] }</h2>
 
           <ol class="sections">
             %for section in chapter['sections']:
@@ -138,8 +138,8 @@ $(function() {
               percentageString = "{0:.0%}".format( float(earned)/total) if earned > 0 and total > 0 else ""
               %>
 
-              <h3><a href="${reverse('courseware_section', kwargs={'course_id' : course.id, 'chapter' : chapter['chapter'], 'section' : section['section']})}">
-                ${ section['section'] }</a> ${"({0:.3n}/{1:.3n}) {2}".format( float(earned), float(total), percentageString )}</h3>
+              <h3><a href="${reverse('courseware_section', kwargs={'course_id' : course.id, 'chapter' : chapter['url_name'], 'section' : section['url_name']})}">
+                ${ section['display_name'] }</a> ${"({0:.3n}/{1:.3n}) {2}".format( float(earned), float(total), percentageString )}</h3>
               ${section['format']}
               %if 'due' in section and section['due']!="":
                 due ${section['due']}

lms/templates/video.html

 % if name is not UNDEFINED and name is not None:
-  <h1> ${name} </h1>
+  <h1> ${display_name} </h1>
 % endif
 
 <div id="video_${id}" class="video" data-streams="${streams}" data-caption-data-dir="${data_dir}">