Track current chapter.

- courseware index view now redirects to most recent chapter, or first
- simplify the view a bit

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

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

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,54 @@ 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(name):
+            """Assumes the course is present"""
+            return [c for c in courses if c.location.course==name][0]
+
+        self.full = find_course("full")
+        self.toy = find_course("toy")
+
+        # 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 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.assertRedirects(resp, reverse('courseware_chapter',
+                                           kwargs={'course_id': self.toy.id, 'chapter': 'secret:magic'}),
+                                           status_code = 302, target_status_code = 200)
+
+
+@override_settings(MODULESTORE=TEST_DATA_XML_MODULESTORE)
 class TestViewAuth(PageLoader):
     """Check that view authentication works properly"""
 
@@ -272,19 +361,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"""

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,55 @@ def render_accordion(request, course, chapter, section, course_id=None):
     return render_to_string('accordion.html', context)
 
 
+def redirect_to_course_position(course_module):
+    """
+    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.
+    """
+    chapters = course_module.get_display_items()
+    # position is 1-indexed.
+    if course_module.position - 1 < len(chapters):
+        chapter = chapters[course_module.position - 1]
+    elif len(chapters) > 0:
+        # Something is wrong.  Default to first chapter.
+        chapter = chapters[0]
+    else:
+        # oops.  Something bad has happened.
+        raise Http404
+
+    return redirect(reverse('courseware_chapter', kwargs={'course_id': course_module.descriptor.id,
+                                                          'chapter': chapter.url_name}))
+
+def save_course_position(course_module, chapter, instance_module):
+    """
+    chapter: url_name of the chapter
+    instance_module: the StudentModule object for the course_module
+    """
+    for i, c in enumerate(course_module.get_display_items()):
+        if c.url_name == chapter:
+            # Position is 1-indexed
+            position = i + 1
+            # Only save if position changed
+            if position != course_module.position:
+                course_module.position = position
+                instance_module.state = course_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 +170,20 @@ 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=1)
+        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 and section is None:
+            return redirect_to_course_position(course_module)
+
         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,28 +191,32 @@ 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))
-        else:
-            if request.user.is_staff:
-                # Add a list of all the errors...
-                context['course_errors'] = modulestore().get_item_errors(course.location)
+        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_course_position(course_module, chapter, instance_module)
+
+        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
+
+            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 may be trying to be clever and access something
+                # they don't have access to.
+                raise Http404
+
+            context['content'] = module.get_html()
+
+        # if request.user.is_staff:
+        #     # Add a list of all the errors...
+        #     context['course_errors'] = modulestore().get_item_errors(course.location)
 
         result = render_to_response('courseware/courseware.html', context)
     except: