Merge pull request #629 from MITx/feature/victor/save-course-position

Feature/victor/save course position

cms/envs/common.py

@@ -35,6 +35,7 @@ from path import path
 MITX_FEATURES = {
     'USE_DJANGO_PIPELINE': True,
     'GITHUB_PUSH': False,
+    'ENABLE_DISCUSSION_SERVICE': False
 }
 
 # needed to use lms student app

common/djangoapps/student/models.py

@@ -282,6 +282,9 @@ def add_user_to_default_group(user, group):
 
 @receiver(post_save, sender=User)
 def update_user_information(sender, instance, created, **kwargs):
+    if not settings.MITX_FEATURES['ENABLE_DISCUSSION_SERVICE']:
+        # Don't try--it won't work, and it will fill the logs with lots of errors
+        return
     try:
         cc_user = cc.User.from_django_user(instance)
         cc_user.save()

common/lib/xmodule/xmodule/seq_module.py

@@ -29,6 +29,8 @@ class SequenceModule(XModule):
                  shared_state=None, **kwargs):
         XModule.__init__(self, system, location, definition, descriptor,
                          instance_state, shared_state, **kwargs)
+        # NOTE: Position is 1-indexed.  This is silly, but there are now student
+        # positions saved on prod, so it's not easy to fix.
         self.position = 1
 
         if instance_state is not None:

common/lib/xmodule/xmodule/x_module.py

@@ -212,7 +212,7 @@ class XModule(HTMLSnippet):
         return self.metadata.get('display_name',
                                  self.url_name.replace('_', ' '))
     def __unicode__(self):
-        return '<x_module(name=%s, category=%s, id=%s)>' % (self.name, self.category, self.id)
+        return '<x_module(id={0})>'.format(self.id)
 
     def get_children(self):
         '''
@@ -465,6 +465,16 @@ class XModuleDescriptor(Plugin, HTMLSnippet):
 
         return self._child_instances
 
+
+    def get_child_by_url_name(self, url_name):
+        """
+        Return a child XModuleDescriptor with the specified url_name, if it exists, and None otherwise.
+        """
+        for c in self.get_children():
+            if c.url_name == url_name:
+                return c
+        return None
+
     def xmodule_constructor(self, system):
         """
         Returns a constructor for an XModule. This constructor takes two

common/test/data/full/course.xml

-<course filename="6.002_Spring_2012" slug="6.002_Spring_2012" graceperiod="1 day 12 hours 59 minutes 59 seconds" showanswer="attempted" rerandomize="never" name="6.002 Spring 2012" start="2015-07-17T12:00" course="full" org="edx"/>
+<course filename="6.002_Spring_2012" slug="6.002_Spring_2012" graceperiod="1 day 12 hours 59 minutes 59 seconds" showanswer="attempted" rerandomize="never" name="6.002 Spring 2012" start="2015-07-17T12:00" course="full" org="edX"/>

lms/djangoapps/courseware/module_render.py

@@ -52,7 +52,7 @@ def make_track_function(request):
     return f
 
 
-def toc_for_course(user, request, course, active_chapter, active_section, course_id=None):
+def toc_for_course(user, request, course, active_chapter, active_section):
     '''
     Create a table of contents from the module store
 
@@ -75,13 +75,13 @@ def toc_for_course(user, request, course, active_chapter, active_section, course
     '''
 
     student_module_cache = StudentModuleCache.cache_for_descriptor_descendents(
-        course_id, user, course, depth=2)
-    course = get_module(user, request, course.location, student_module_cache, course_id)
-    if course is None:
+        course.id, user, course, depth=2)
+    course_module = get_module(user, request, course.location, student_module_cache, course.id)
+    if course_module is None:
         return None
 
     chapters = list()
-    for chapter in course.get_display_items():
+    for chapter in course_module.get_display_items():
         hide_from_toc = chapter.metadata.get('hide_from_toc','false').lower() == 'true'
         if hide_from_toc:
             continue
@@ -109,36 +109,6 @@ def toc_for_course(user, request, course, active_chapter, active_section, course
     return chapters
 
 
-def get_section(course_module, chapter, section):
-    """
-    Returns the xmodule descriptor for the name course > chapter > section,
-    or None if this doesn't specify a valid section
-
-    course: Course url
-    chapter: Chapter url_name
-    section: Section url_name
-    """
-
-    if course_module is None:
-        return
-
-    chapter_module = None
-    for _chapter in course_module.get_children():
-        if _chapter.url_name == chapter:
-            chapter_module = _chapter
-            break
-
-    if chapter_module is None:
-        return
-
-    section_module = None
-    for _section in chapter_module.get_children():
-        if _section.url_name == section:
-            section_module = _section
-            break
-
-    return section_module
-
 def get_module(user, request, location, student_module_cache, course_id, position=None):
     """
     Get an instance of the xmodule class identified by location,
@@ -293,9 +263,10 @@ def _get_module(user, request, location, student_module_cache, course_id, positi
 
     return module
 
+# TODO (vshnayder): Rename this?  It's very confusing.
 def get_instance_module(course_id, user, module, student_module_cache):
     """
-    Returns instance_module is a StudentModule specific to this module for this student,
+    Returns the StudentModule specific to this module for this student,
         or None if this is an anonymous user
     """
     if user.is_authenticated():

lms/djangoapps/courseware/tests/tests.py

@@ -7,6 +7,7 @@ import time
 from nose import SkipTest
 from path import path
 from pprint import pprint
+from urlparse import urlsplit, urlunsplit
 
 from django.contrib.auth.models import User, Group
 from django.test import TestCase
@@ -83,6 +84,27 @@ REAL_DATA_MODULESTORE = mongo_store_config(REAL_DATA_DIR)
 class ActivateLoginTestCase(TestCase):
     '''Check that we can activate and log in'''
 
+    def assertRedirectsNoFollow(self, response, expected_url):
+        """
+        http://devblog.point2.com/2010/04/23/djangos-assertredirects-little-gotcha/
+
+        Don't check that the redirected-to page loads--there should be other tests for that.
+
+        Some of the code taken from django.test.testcases.py
+        """
+        self.assertEqual(response.status_code, 302,
+                         'Response status code was {0} instead of 302'.format(response.status_code))
+        url = response['Location']
+
+        e_scheme, e_netloc, e_path, e_query, e_fragment = urlsplit(
+                                                              expected_url)
+        if not (e_scheme or e_netloc):
+            expected_url = urlunsplit(('http', 'testserver', e_path,
+                e_query, e_fragment))
+
+        self.assertEqual(url, expected_url, "Response redirected to '{0}', expected '{1}'".format(
+            url, expected_url))
+
     def setUp(self):
         email = 'view@test.com'
         password = 'foo'
@@ -193,6 +215,25 @@ class PageLoader(ActivateLoginTestCase):
         data = parse_json(resp)
         self.assertTrue(data['success'])
 
+
+    def check_for_get_code(self, code, url):
+        """
+        Check that we got the expected code.  Hacks around our broken 404
+        handling.
+        """
+        resp = self.client.get(url)
+        # HACK: workaround the bug that returns 200 instead of 404.
+        # TODO (vshnayder): once we're returning 404s, get rid of this if.
+        if code != 404:
+            self.assertEqual(resp.status_code, code)
+            # And 'page not found' shouldn't be in the returned page
+            self.assertTrue(resp.content.lower().find('page not found') == -1)
+        else:
+            # look for "page not found" instead of the status code
+            #print resp.content
+            self.assertTrue(resp.content.lower().find('page not found') != -1)
+
+
     def check_pages_load(self, course_name, data_dir, modstore):
         """Make all locations in course load"""
         print "Checking course {0} in {1}".format(course_name, data_dir)
@@ -204,7 +245,7 @@ class PageLoader(ActivateLoginTestCase):
         course = courses[0]
         self.enroll(course)
         course_id = course.id
-        
+
         n = 0
         num_bad = 0
         all_ok = True
@@ -246,6 +287,61 @@ class TestCoursesLoadTestCase(PageLoader):
 
 
 @override_settings(MODULESTORE=TEST_DATA_XML_MODULESTORE)
+class TestNavigation(PageLoader):
+    """Check that navigation state is saved properly"""
+
+    def setUp(self):
+        xmodule.modulestore.django._MODULESTORES = {}
+        courses = modulestore().get_courses()
+
+        def find_course(course_id):
+            """Assumes the course is present"""
+            return [c for c in courses if c.id==course_id][0]
+
+        self.full = find_course("edX/full/6.002_Spring_2012")
+        self.toy = find_course("edX/toy/2012_Fall")
+
+        # Create two accounts
+        self.student = 'view@test.com'
+        self.student2 = 'view2@test.com'
+        self.password = 'foo'
+        self.create_account('u1', self.student, self.password)
+        self.create_account('u2', self.student2, self.password)
+        self.activate_user(self.student)
+        self.activate_user(self.student2)
+
+    def test_accordion_state(self):
+        """Make sure that the accordion remembers where you were properly"""
+        self.login(self.student, self.password)
+        self.enroll(self.toy)
+        self.enroll(self.full)
+
+        # First request should redirect to ToyVideos
+        resp = self.client.get(reverse('courseware', kwargs={'course_id': self.toy.id}))
+
+        # Don't use no-follow, because state should only be saved once we actually hit the section
+        self.assertRedirects(resp, reverse(
+            'courseware_section', kwargs={'course_id': self.toy.id,
+                                          'chapter': 'Overview',
+                                          'section': 'Toy_Videos'}))
+
+        # Hitting the couseware tab again should redirect to the first chapter: 'Overview'
+        resp = self.client.get(reverse('courseware', kwargs={'course_id': self.toy.id}))
+        self.assertRedirectsNoFollow(resp, reverse('courseware_chapter',
+                                                   kwargs={'course_id': self.toy.id, 'chapter': 'Overview'}))
+
+        # Now we directly navigate to a section in a different chapter
+        self.check_for_get_code(200, reverse('courseware_section',
+                                             kwargs={'course_id': self.toy.id,
+                                                     'chapter':'secret:magic', 'section':'toyvideo'}))
+
+        # And now hitting the courseware tab should redirect to 'secret:magic'
+        resp = self.client.get(reverse('courseware', kwargs={'course_id': self.toy.id}))
+        self.assertRedirectsNoFollow(resp, reverse('courseware_chapter',
+                                                   kwargs={'course_id': self.toy.id, 'chapter': 'secret:magic'}))
+
+
+@override_settings(MODULESTORE=TEST_DATA_XML_MODULESTORE)
 class TestViewAuth(PageLoader):
     """Check that view authentication works properly"""
 
@@ -256,12 +352,12 @@ class TestViewAuth(PageLoader):
         xmodule.modulestore.django._MODULESTORES = {}
         courses = modulestore().get_courses()
 
-        def find_course(name):
+        def find_course(course_id):
             """Assumes the course is present"""
-            return [c for c in courses if c.location.course==name][0]
+            return [c for c in courses if c.id==course_id][0]
 
-        self.full = find_course("full")
-        self.toy = find_course("toy")
+        self.full = find_course("edX/full/6.002_Spring_2012")
+        self.toy = find_course("edX/toy/2012_Fall")
 
         # Create two accounts
         self.student = 'view@test.com'
@@ -272,19 +368,6 @@ class TestViewAuth(PageLoader):
         self.activate_user(self.student)
         self.activate_user(self.instructor)
 
-    def check_for_get_code(self, code, url):
-        resp = self.client.get(url)
-        # HACK: workaround the bug that returns 200 instead of 404.
-        # TODO (vshnayder): once we're returning 404s, get rid of this if.
-        if code != 404:
-            self.assertEqual(resp.status_code, code)
-            # And 'page not found' shouldn't be in the returned page
-            self.assertTrue(resp.content.lower().find('page not found') == -1)
-        else:
-            # look for "page not found" instead of the status code
-            #print resp.content
-            self.assertTrue(resp.content.lower().find('page not found') != -1)
-
     def test_instructor_pages(self):
         """Make sure only instructors for the course or staff can load the instructor
         dashboard, the grade views, and student profile pages"""
@@ -292,11 +375,15 @@ class TestViewAuth(PageLoader):
         # First, try with an enrolled student
         self.login(self.student, self.password)
         # shouldn't work before enroll
-        self.check_for_get_code(302, reverse('courseware', kwargs={'course_id': self.toy.id}))
+        response = self.client.get(reverse('courseware', kwargs={'course_id': self.toy.id}))
+        self.assertRedirectsNoFollow(response, reverse('about_course', args=[self.toy.id]))
         self.enroll(self.toy)
         self.enroll(self.full)
-        # should work now
-        self.check_for_get_code(200, reverse('courseware', kwargs={'course_id': self.toy.id}))
+        # should work now -- redirect to first page
+        response = self.client.get(reverse('courseware', kwargs={'course_id': self.toy.id}))
+        self.assertRedirectsNoFollow(response, reverse('courseware_section', kwargs={'course_id': self.toy.id,
+                                                                                     'chapter': 'Overview',
+                                                                                     'section': 'Toy_Videos'}))
 
         def instructor_urls(course):
             "list of urls that only instructors/staff should be able to see"
@@ -389,7 +476,7 @@ class TestViewAuth(PageLoader):
             list of urls that students should be able to see only
             after launch, but staff should see before
             """
-            urls = reverse_urls(['info', 'courseware', 'progress'], course)
+            urls = reverse_urls(['info', 'progress'], course)
             urls.extend([
                 reverse('book', kwargs={'course_id': course.id, 'book_index': book.title})
                 for book in course.textbooks
@@ -417,7 +504,7 @@ class TestViewAuth(PageLoader):
         def check_non_staff(course):
             """Check that access is right for non-staff in course"""
             print '=== Checking non-staff access for {0}'.format(course.id)
-            for url in instructor_urls(course) + dark_student_urls(course):
+            for url in instructor_urls(course) + dark_student_urls(course) + reverse_urls(['courseware'], course):
                 print 'checking for 404 on {0}'.format(url)
                 self.check_for_get_code(404, url)
 
@@ -444,6 +531,10 @@ class TestViewAuth(PageLoader):
             print 'checking for 404 on view-as-student: {0}'.format(url)
             self.check_for_get_code(404, url)
 
+            # The courseware url should redirect, not 200
+            url = reverse_urls(['courseware'], course)[0]
+            self.check_for_get_code(302, url)
+
 
         # First, try with an enrolled student
         print '=== Testing student access....'

lms/djangoapps/courseware/views.py

@@ -23,7 +23,7 @@ from courseware import grades
 from courseware.access import has_access
 from courseware.courses import (get_course_with_access, get_courses_by_university)
 from models import StudentModuleCache
-from module_render import toc_for_course, get_module, get_section
+from module_render import toc_for_course, get_module, get_instance_module
 from student.models import UserProfile
 
 from multicourse import multicourse_settings
@@ -40,9 +40,6 @@ from xmodule.modulestore.search import path_to_location
 
 import comment_client
 
-
-
-
 log = logging.getLogger("mitx.courseware")
 
 template_imports = {'urllib': urllib}
@@ -83,7 +80,7 @@ def courses(request):
     return render_to_response("courses.html", {'universities': universities})
 
 
-def render_accordion(request, course, chapter, section, course_id=None):
+def render_accordion(request, course, chapter, section):
     ''' Draws navigation bar. Takes current position in accordion as
         parameter.
 
@@ -94,7 +91,7 @@ def render_accordion(request, course, chapter, section, course_id=None):
         Returns the html string'''
 
     # grab the table of contents
-    toc = toc_for_course(request.user, request, course, chapter, section, course_id=course_id)
+    toc = toc_for_course(request.user, request, course, chapter, section)
 
     context = dict([('toc', toc),
                     ('course_id', course.id),
@@ -102,16 +99,78 @@ def render_accordion(request, course, chapter, section, course_id=None):
     return render_to_string('accordion.html', context)
 
 
+def get_current_child(xmodule):
+    """
+    Get the xmodule.position's display item of an xmodule that has a position and
+    children.  Returns None if the xmodule doesn't have a position, or if there
+    are no children.  Otherwise, if position is out of bounds, returns the first child.
+    """
+    if not hasattr(xmodule, 'position'):
+        return None
+
+    children = xmodule.get_display_items()
+    # position is 1-indexed.
+    if 0 <= xmodule.position - 1 < len(children):
+        child = children[xmodule.position - 1]
+    elif len(children) > 0:
+        # Something is wrong.  Default to first child
+        child = children[0]
+    else:
+        child = None
+    return child
+
+
+def redirect_to_course_position(course_module, first_time):
+    """
+    Load the course state for the user, and return a redirect to the
+    appropriate place in the course: either the first element if there
+    is no state, or their previous place if there is.
+
+    If this is the user's first time, send them to the first section instead.
+    """
+    course_id = course_module.descriptor.id
+    chapter = get_current_child(course_module)
+    if chapter is None:
+        # oops.  Something bad has happened.
+        raise Http404
+    if not first_time:
+        return redirect(reverse('courseware_chapter', kwargs={'course_id': course_id,
+                                                              'chapter': chapter.url_name}))
+    # Relying on default of returning first child
+    section = get_current_child(chapter)
+    return redirect(reverse('courseware_section', kwargs={'course_id': course_id,
+                                                          'chapter': chapter.url_name,
+                                                          'section': section.url_name}))
+
+def save_child_position(seq_module, child_name, instance_module):
+    """
+    child_name: url_name of the child
+    instance_module: the StudentModule object for the seq_module
+    """
+    for i, c in enumerate(seq_module.get_display_items()):
+        if c.url_name == child_name:
+            # Position is 1-indexed
+            position = i + 1
+            # Only save if position changed
+            if position != seq_module.position:
+                seq_module.position = position
+                instance_module.state = seq_module.get_instance_state()
+                instance_module.save()
+
 @login_required
 @ensure_csrf_cookie
 @cache_control(no_cache=True, no_store=True, must_revalidate=True)
 def index(request, course_id, chapter=None, section=None,
           position=None):
     """
-    Displays courseware accordion, and any associated content.
-    If course, chapter, and section aren't all specified, just returns
-    the accordion.  If they are specified, returns an error if they don't
-    point to a valid module.
+    Displays courseware accordion and associated content.  If course, chapter,
+    and section are all specified, renders the page, or returns an error if they
+    are invalid.
+
+    If section is not specified, displays the accordion opened to the right chapter.
+
+    If neither chapter or section are specified, redirects to user's most recent
+    chapter, or the first chapter if this is the user's first visit.
 
     Arguments:
 
@@ -134,9 +193,24 @@ def index(request, course_id, chapter=None, section=None,
         return redirect(reverse('about_course', args=[course.id]))
 
     try:
+        student_module_cache = StudentModuleCache.cache_for_descriptor_descendents(
+            course.id, request.user, course, depth=2)
+
+        # Has this student been in this course before?
+        first_time = student_module_cache.lookup(course_id, 'course', course.location.url()) is None
+
+        course_module = get_module(request.user, request, course.location, student_module_cache, course.id)
+        if course_module is None:
+            log.warning('If you see this, something went wrong: if we got this'
+                        ' far, should have gotten a course module for this user')
+            return redirect(reverse('about_course', args=[course.id]))
+
+        if chapter is None:
+            return redirect_to_course_position(course_module, first_time)
+
         context = {
             'csrf': csrf(request)['csrf_token'],
-            'accordion': render_accordion(request, course, chapter, section, course_id=course_id),
+            'accordion': render_accordion(request, course, chapter, section),
             'COURSE_TITLE': course.title,
             'course': course,
             'init': '',
@@ -144,31 +218,62 @@ def index(request, course_id, chapter=None, section=None,
             'staff_access': staff_access,
             }
 
-        look_for_module = chapter is not None and section is not None
-        if look_for_module:
-            section_descriptor = get_section(course, chapter, section)
-            if section_descriptor is not None:
-                student_module_cache = StudentModuleCache.cache_for_descriptor_descendents(
-                    course_id, request.user, section_descriptor)
-                module = get_module(request.user, request,
-                                    section_descriptor.location,
-                                    student_module_cache, course_id, position)
-                if module is None:
-                    # User is probably being clever and trying to access something
-                    # they don't have access to.
-                    raise Http404
-                context['content'] = module.get_html()
-            else:
-                log.warning("Couldn't find a section descriptor for course_id '{0}',"
-                            "chapter '{1}', section '{2}'".format(
-                                course_id, chapter, section))
+        chapter_descriptor = course.get_child_by_url_name(chapter)
+        if chapter_descriptor is not None:
+            instance_module = get_instance_module(course_id, request.user, course_module, student_module_cache)
+            save_child_position(course_module, chapter, instance_module)
         else:
-            if request.user.is_staff:
-                # Add a list of all the errors...
-                context['course_errors'] = modulestore().get_item_errors(course.location)
+            raise Http404
+
+        chapter_module = get_module(request.user, request, chapter_descriptor.location,
+                                    student_module_cache, course_id)
+        if chapter_module is None:
+            # User may be trying to access a chapter that isn't live yet
+            raise Http404
+
+        if section is not None:
+            section_descriptor = chapter_descriptor.get_child_by_url_name(section)
+            if section_descriptor is None:
+                # Specifically asked-for section doesn't exist
+                raise Http404
+
+            section_student_module_cache = StudentModuleCache.cache_for_descriptor_descendents(
+                course_id, request.user, section_descriptor)
+            section_module = get_module(request.user, request,
+                                section_descriptor.location,
+                                section_student_module_cache, course_id, position)
+            if section_module is None:
+                # User may be trying to be clever and access something
+                # they don't have access to.
+                raise Http404
+
+            # Save where we are in the chapter
+            instance_module = get_instance_module(course_id, request.user, chapter_module, student_module_cache)
+            save_child_position(chapter_module, section, instance_module)
+
+
+            context['content'] = section_module.get_html()
+        else:
+            # section is none, so display a message
+            prev_section = get_current_child(chapter_module)
+            if prev_section is None:
+                # Something went wrong -- perhaps this chapter has no sections visible to the user
+                raise Http404
+            prev_section_url = reverse('courseware_section', kwargs={'course_id': course_id,
+                                                                     'chapter': chapter_descriptor.url_name,
+                                                                     'section': prev_section.url_name})
+            context['content'] = render_to_string('courseware/welcome-back.html',
+                                                  {'course': course,
+                                                   'chapter_module': chapter_module,
+                                                   'prev_section': prev_section,
+                                                   'prev_section_url': prev_section_url})
 
         result = render_to_response('courseware/courseware.html', context)
-    except:
+    except Exception as e:
+        if isinstance(e, Http404):
+            # let it propagate
+            raise
+
         # In production, don't want to let a 500 out for any reason
         if settings.DEBUG:
             raise

lms/djangoapps/instructor/views.py

@@ -194,6 +194,7 @@ def instructor_dashboard(request, course_id):
                'instructor_access': instructor_access,
                'datatable': datatable,
                'msg': msg,
+               'course_errors': modulestore().get_item_errors(course.location),
                }
 
     return render_to_response('courseware/instructor_dashboard.html', context)

lms/envs/test.py

@@ -16,6 +16,9 @@ from path import path
 # can test everything else :)
 MITX_FEATURES['DISABLE_START_DATES'] = True
 
+# Until we have discussion actually working in test mode, just turn it off
+MITX_FEATURES['ENABLE_DISCUSSION_SERVICE'] = False
+
 # Need wiki for courseware views to work. TODO (vshnayder): shouldn't need it.
 WIKI_ENABLED = True
 

lms/templates/courseware/courseware.html

@@ -56,19 +56,6 @@
 
     <section class="course-content">
       ${content}
-
-      % if course_errors is not UNDEFINED:
-          <h2>Course errors</h2>
-          <div id="course-errors">
-            <ul>
-          % for (msg, err) in course_errors:
-            <li>${msg | h}
-              <ul><li><pre>${err | h}</pre></li></ul>
-            </li>
-          % endfor
-            </ul>
-          </div>
-      % endif
     </section>
   </div>
 </section>

lms/templates/courseware/instructor_dashboard.html

@@ -104,6 +104,19 @@ table.stat_table td {
     <p>${msg}</p>    
 %endif
 
+% if course_errors is not UNDEFINED:
+    <h2>Course errors</h2>
+    <div id="course-errors">
+      <ul>
+        % for (summary, err) in course_errors:
+        <li>${summary | h}
+          <ul><li><pre>${err | h}</pre></li></ul>
+        </li>
+        % endfor
+        </ul>
+      </div>
+% endif
+      
   </section>
 </div>
 </section>

lms/templates/courseware/welcome-back.html

+<h2>${chapter_module.display_name}</h2>
+
+<p>You were last in <a href="${prev_section_url}">${prev_section.display_name}</a>.  If you're done with that, choose another section on the left.</p> 
\ No newline at end of file