diff --git a/common/lib/xmodule/xmodule/seq_module.py b/common/lib/xmodule/xmodule/seq_module.py index fee4d53700..65f692957c 100644 --- a/common/lib/xmodule/xmodule/seq_module.py +++ b/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: diff --git a/common/lib/xmodule/xmodule/x_module.py b/common/lib/xmodule/xmodule/x_module.py index 000d64e60b..e21e858baa 100644 --- a/common/lib/xmodule/xmodule/x_module.py +++ b/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 diff --git a/lms/djangoapps/courseware/module_render.py b/lms/djangoapps/courseware/module_render.py index b8416426b6..88368c4a80 100644 --- a/lms/djangoapps/courseware/module_render.py +++ b/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(): diff --git a/lms/djangoapps/courseware/tests/tests.py b/lms/djangoapps/courseware/tests/tests.py index d7fffb7499..0d295f5c0f 100644 --- a/lms/djangoapps/courseware/tests/tests.py +++ b/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 @@ -245,6 +286,54 @@ class TestCoursesLoadTestCase(PageLoader): self.check_pages_load('full', TEST_DATA_DIR, modulestore()) +@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""" diff --git a/lms/djangoapps/courseware/views.py b/lms/djangoapps/courseware/views.py index 60279d34c9..e0c73f1b4b 100644 --- a/lms/djangoapps/courseware/views.py +++ b/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: