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

Feature/victor/save course position

cms/envs/common.py

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

common/djangoapps/student/models.py

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

common/lib/xmodule/xmodule/seq_module.py

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

common/lib/xmodule/xmodule/x_module.py

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

common/test/data/full/course.xml

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

lms/djangoapps/courseware/module_render.py

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

lms/djangoapps/courseware/tests/tests.py

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

lms/djangoapps/instructor/views.py

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

lms/envs/test.py

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

lms/templates/courseware/courseware.html

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

lms/templates/courseware/instructor_dashboard.html

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

lms/templates/courseware/welcome-back.html

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