diff --git a/cms/envs/common.py b/cms/envs/common.py index 1767202141..f1440c7292 100644 --- a/cms/envs/common.py +++ b/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 diff --git a/common/djangoapps/student/models.py b/common/djangoapps/student/models.py index c37b9fce16..7e8658045e 100644 --- a/common/djangoapps/student/models.py +++ b/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() 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 be61498de6..e21e858baa 100644 --- a/common/lib/xmodule/xmodule/x_module.py +++ b/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 '' % (self.name, self.category, self.id) + return ''.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 diff --git a/common/test/data/full/course.xml b/common/test/data/full/course.xml index c365e68cc1..4f093a1128 100644 --- a/common/test/data/full/course.xml +++ b/common/test/data/full/course.xml @@ -1 +1 @@ - + 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..62456d65d5 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,61 @@ 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(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....' diff --git a/lms/djangoapps/courseware/views.py b/lms/djangoapps/courseware/views.py index 60279d34c9..c474da8d8b 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,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 diff --git a/lms/djangoapps/instructor/views.py b/lms/djangoapps/instructor/views.py index 47ae88510c..f4e9c27991 100644 --- a/lms/djangoapps/instructor/views.py +++ b/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) diff --git a/lms/envs/test.py b/lms/envs/test.py index 7cab4cb52c..b55cf2af5f 100644 --- a/lms/envs/test.py +++ b/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 diff --git a/lms/templates/courseware/courseware.html b/lms/templates/courseware/courseware.html index 0d390d83a2..29b9be3789 100644 --- a/lms/templates/courseware/courseware.html +++ b/lms/templates/courseware/courseware.html @@ -56,19 +56,6 @@
${content} - - % if course_errors is not UNDEFINED: -

Course errors

-
-
    - % for (msg, err) in course_errors: -
  • ${msg | h} - 
    • ${err | h}
    -
    • - % endfor -
-
- % endif
diff --git a/lms/templates/courseware/instructor_dashboard.html b/lms/templates/courseware/instructor_dashboard.html index b927302bed..2d9ab853eb 100644 --- a/lms/templates/courseware/instructor_dashboard.html +++ b/lms/templates/courseware/instructor_dashboard.html @@ -104,6 +104,19 @@ table.stat_table td {

${msg}

%endif +% if course_errors is not UNDEFINED: +

Course errors

+
+
    + % for (summary, err) in course_errors: +
  • ${summary | h} + 
    • ${err | h}
    +
    • + % endfor +
+
+% endif + diff --git a/lms/templates/courseware/welcome-back.html b/lms/templates/courseware/welcome-back.html new file mode 100644 index 0000000000..a817d303a9 --- /dev/null +++ b/lms/templates/courseware/welcome-back.html @@ -0,0 +1,3 @@ +

${chapter_module.display_name}

+ +

You were last in ${prev_section.display_name}. If you're done with that, choose another section on the left.

\ No newline at end of file