From 6cfe52680bb781873e920586eacec3f92c14fccd Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Wed, 5 Sep 2012 12:53:14 -0400 Subject: [PATCH 01/13] Fix XModule __unicode__ method - was referencing no-longer-present attributes --- common/lib/xmodule/xmodule/x_module.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/common/lib/xmodule/xmodule/x_module.py b/common/lib/xmodule/xmodule/x_module.py index be61498de6..000d64e60b 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): ''' From 4481adb0413dd143764ac87c9207b99858d4f131 Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Tue, 4 Sep 2012 00:16:15 -0400 Subject: [PATCH 02/13] 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 | 2 + common/lib/xmodule/xmodule/x_module.py | 10 ++ lms/djangoapps/courseware/module_render.py | 43 ++------ lms/djangoapps/courseware/tests/tests.py | 104 +++++++++++++++--- lms/djangoapps/courseware/views.py | 117 +++++++++++++++------ 5 files changed, 193 insertions(+), 83 deletions(-) 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: From c354a120d8cdc2770a6c9f669a27d62b2616019b Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Wed, 5 Sep 2012 15:53:59 -0400 Subject: [PATCH 03/13] Track accordion state: - on first visit to courseware, go straight to first section of first chapter - after, clicking on courseware tab sends you most recent chapter, with a link to the most recent section (not to section because that might be confusing, and you might want to do something else (e.g. do homework instead of watch videos) - Moved course errors into instructor tab. --- lms/djangoapps/courseware/views.py | 107 +++++++++++++----- lms/djangoapps/instructor/views.py | 1 + lms/templates/courseware/courseware.html | 13 --- .../courseware/instructor_dashboard.html | 13 +++ lms/templates/courseware/welcome-back.html | 3 + 5 files changed, 93 insertions(+), 44 deletions(-) create mode 100644 lms/templates/courseware/welcome-back.html diff --git a/lms/djangoapps/courseware/views.py b/lms/djangoapps/courseware/views.py index e0c73f1b4b..105d4cea9e 100644 --- a/lms/djangoapps/courseware/views.py +++ b/lms/djangoapps/courseware/views.py @@ -99,39 +99,62 @@ def render_accordion(request, course, chapter, section): return render_to_string('accordion.html', context) -def redirect_to_course_position(course_module): +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. """ - 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: + 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})) - 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): +def save_child_position(seq_module, child_name, instance_module): """ - chapter: url_name of the chapter - instance_module: the StudentModule object for the course_module + child_name: url_name of the child + instance_module: the StudentModule object for the seq_module """ - for i, c in enumerate(course_module.get_display_items()): - if c.url_name == chapter: + 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 != course_module.position: - course_module.position = position - instance_module.state = course_module.get_instance_state() + if position != seq_module.position: + seq_module.position = position + instance_module.state = seq_module.get_instance_state() instance_module.save() @login_required @@ -171,7 +194,11 @@ def index(request, course_id, chapter=None, section=None, try: student_module_cache = StudentModuleCache.cache_for_descriptor_descendents( - course.id, request.user, course, depth=1) + 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' @@ -179,7 +206,7 @@ def index(request, course_id, chapter=None, section=None, return redirect(reverse('about_course', args=[course.id])) if chapter is None and section is None: - return redirect_to_course_position(course_module) + return redirect_to_course_position(course_module, first_time) context = { 'csrf': csrf(request)['csrf_token'], @@ -194,7 +221,10 @@ def index(request, course_id, chapter=None, section=None, 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) + save_child_position(course_module, chapter, instance_module) + + chapter_module = get_module(request.user, request, chapter_descriptor.location, + student_module_cache, course_id) if section is not None: section_descriptor = chapter_descriptor.get_child_by_url_name(section) @@ -202,21 +232,36 @@ def index(request, course_id, chapter=None, section=None, # Specifically asked-for section doesn't exist raise Http404 - student_module_cache = StudentModuleCache.cache_for_descriptor_descendents( + section_student_module_cache = StudentModuleCache.cache_for_descriptor_descendents( course_id, request.user, section_descriptor) - module = get_module(request.user, request, + section_module = get_module(request.user, request, section_descriptor.location, - student_module_cache, course_id, position) - if module is None: + 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 - context['content'] = module.get_html() + # 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) - # if request.user.is_staff: - # # Add a list of all the errors... - # context['course_errors'] = modulestore().get_item_errors(course.location) + + 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: 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/templates/courseware/courseware.html b/lms/templates/courseware/courseware.html index 2950aa827e..6d7fa5193c 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 From 5738a8cdbb35cfe959d1089b605224f4e7f040ea Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Wed, 5 Sep 2012 16:15:48 -0400 Subject: [PATCH 04/13] update tests --- lms/djangoapps/courseware/tests/tests.py | 24 +++++++++++++++++------- 1 file changed, 17 insertions(+), 7 deletions(-) diff --git a/lms/djangoapps/courseware/tests/tests.py b/lms/djangoapps/courseware/tests/tests.py index 0d295f5c0f..65a234aa41 100644 --- a/lms/djangoapps/courseware/tests/tests.py +++ b/lms/djangoapps/courseware/tests/tests.py @@ -316,11 +316,13 @@ class TestNavigation(PageLoader): self.enroll(self.toy) self.enroll(self.full) - # First request should redirect to Overview + # First request should redirect to ToyVideos 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'})) + 'courseware_section', kwargs={'course_id': self.toy.id, + 'chapter': 'Overview', + 'section': 'Toy_Videos'})) # Now we directly navigate to a section in a different chapter self.check_for_get_code(200, reverse('courseware_section', @@ -368,11 +370,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" @@ -465,7 +471,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 @@ -493,7 +499,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) @@ -520,6 +526,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....' From 7c34b02e3bfba003da0116ecc1f6a416cc851d94 Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Wed, 5 Sep 2012 17:25:16 -0400 Subject: [PATCH 05/13] use a NoFollow redirect check --- lms/djangoapps/courseware/tests/tests.py | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/lms/djangoapps/courseware/tests/tests.py b/lms/djangoapps/courseware/tests/tests.py index 65a234aa41..92c09187f6 100644 --- a/lms/djangoapps/courseware/tests/tests.py +++ b/lms/djangoapps/courseware/tests/tests.py @@ -331,9 +331,8 @@ class TestNavigation(PageLoader): # 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) + self.assertRedirectsNoFollow(resp, reverse('courseware_chapter', + kwargs={'course_id': self.toy.id, 'chapter': 'secret:magic'})) @override_settings(MODULESTORE=TEST_DATA_XML_MODULESTORE) From 91d0fe8a1b3cc20b284f38386a605d67fa5e5ceb Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Thu, 6 Sep 2012 10:25:56 -0400 Subject: [PATCH 06/13] extend test to cover first-chapter state --- lms/djangoapps/courseware/tests/tests.py | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/lms/djangoapps/courseware/tests/tests.py b/lms/djangoapps/courseware/tests/tests.py index 92c09187f6..a62e9ab0cd 100644 --- a/lms/djangoapps/courseware/tests/tests.py +++ b/lms/djangoapps/courseware/tests/tests.py @@ -319,11 +319,17 @@ class TestNavigation(PageLoader): # First request should redirect to ToyVideos resp = self.client.get(reverse('courseware', kwargs={'course_id': self.toy.id})) - self.assertRedirectsNoFollow(resp, reverse( + # 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, From 6ab80fb4a85b6aa95d639c57dd8058f48187d623 Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Thu, 6 Sep 2012 12:06:51 -0400 Subject: [PATCH 07/13] debug msg to diagnose jenkins --- lms/djangoapps/courseware/views.py | 3 +++ 1 file changed, 3 insertions(+) diff --git a/lms/djangoapps/courseware/views.py b/lms/djangoapps/courseware/views.py index 105d4cea9e..897775cc4e 100644 --- a/lms/djangoapps/courseware/views.py +++ b/lms/djangoapps/courseware/views.py @@ -205,6 +205,9 @@ def index(request, course_id, chapter=None, section=None, ' far, should have gotten a course module for this user') return redirect(reverse('about_course', args=[course.id])) + log.debug("TEMP: course_id {}, chap {}, sec {}, first_time {}, course position = {}" + .format(course_id, chapter, section, first_time, course_module.position)) + if chapter is None and section is None: return redirect_to_course_position(course_module, first_time) From d4c0516c8b4f0b441242f9db27661defdd6251fa Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Thu, 6 Sep 2012 14:41:09 -0400 Subject: [PATCH 08/13] another attempt to see what's broken on jenkins --- lms/djangoapps/courseware/views.py | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/lms/djangoapps/courseware/views.py b/lms/djangoapps/courseware/views.py index 897775cc4e..ec96a38d3d 100644 --- a/lms/djangoapps/courseware/views.py +++ b/lms/djangoapps/courseware/views.py @@ -205,7 +205,7 @@ def index(request, course_id, chapter=None, section=None, ' far, should have gotten a course module for this user') return redirect(reverse('about_course', args=[course.id])) - log.debug("TEMP: course_id {}, chap {}, sec {}, first_time {}, course position = {}" + log.warning("TEMP: course_id {}, chap {}, sec {}, first_time {}, course position = {}" .format(course_id, chapter, section, first_time, course_module.position)) if chapter is None and section is None: From 02ddeed72a4c17c490271cd51fa4df9237199346 Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Thu, 6 Sep 2012 16:24:59 -0400 Subject: [PATCH 09/13] better error handling in index view --- lms/djangoapps/courseware/views.py | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/lms/djangoapps/courseware/views.py b/lms/djangoapps/courseware/views.py index ec96a38d3d..334b335d26 100644 --- a/lms/djangoapps/courseware/views.py +++ b/lms/djangoapps/courseware/views.py @@ -208,7 +208,7 @@ def index(request, course_id, chapter=None, section=None, log.warning("TEMP: course_id {}, chap {}, sec {}, first_time {}, course position = {}" .format(course_id, chapter, section, first_time, course_module.position)) - if chapter is None and section is None: + if chapter is None: return redirect_to_course_position(course_module, first_time) context = { @@ -225,9 +225,14 @@ def index(request, course_id, chapter=None, section=None, 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: + 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) @@ -267,7 +272,11 @@ def index(request, course_id, chapter=None, section=None, '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 From 048dea0eff652b5fc7378a5e8f416a928ab3f3b6 Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Thu, 6 Sep 2012 16:38:33 -0400 Subject: [PATCH 10/13] make test course loading deterministic by using course_id --- common/test/data/full/course.xml | 2 +- lms/djangoapps/courseware/tests/tests.py | 16 ++++++++-------- 2 files changed, 9 insertions(+), 9 deletions(-) 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/tests/tests.py b/lms/djangoapps/courseware/tests/tests.py index a62e9ab0cd..62456d65d5 100644 --- a/lms/djangoapps/courseware/tests/tests.py +++ b/lms/djangoapps/courseware/tests/tests.py @@ -294,12 +294,12 @@ class TestNavigation(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' @@ -352,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' From 8b7390c9665045c4fa1a9cc1d8f3e277daff438e Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Thu, 6 Sep 2012 17:11:21 -0400 Subject: [PATCH 11/13] Turn of discussion service in test env - add a check for the feature flag before trying to save user info to the service --- common/djangoapps/student/models.py | 3 +++ lms/envs/test.py | 3 +++ 2 files changed, 6 insertions(+) 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/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 From 6017aa698b295ab8e6d3445c3f785fa645119a1c Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Thu, 6 Sep 2012 17:28:12 -0400 Subject: [PATCH 12/13] add ENABLE_DISCUSSION_SERVICE: "False" to cms config --- cms/envs/common.py | 1 + 1 file changed, 1 insertion(+) 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 From 1e94ff19f3bbc884de971b103c4551e44d7ec8df Mon Sep 17 00:00:00 2001 From: Victor Shnayder Date: Fri, 7 Sep 2012 08:20:59 -0400 Subject: [PATCH 13/13] remove debuging log message --- lms/djangoapps/courseware/views.py | 5 +---- 1 file changed, 1 insertion(+), 4 deletions(-) diff --git a/lms/djangoapps/courseware/views.py b/lms/djangoapps/courseware/views.py index 334b335d26..c474da8d8b 100644 --- a/lms/djangoapps/courseware/views.py +++ b/lms/djangoapps/courseware/views.py @@ -205,9 +205,6 @@ def index(request, course_id, chapter=None, section=None, ' far, should have gotten a course module for this user') return redirect(reverse('about_course', args=[course.id])) - log.warning("TEMP: course_id {}, chap {}, sec {}, first_time {}, course position = {}" - .format(course_id, chapter, section, first_time, course_module.position)) - if chapter is None: return redirect_to_course_position(course_module, first_time) @@ -276,7 +273,7 @@ def index(request, course_id, chapter=None, section=None, 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