Merge pull request #3754 from theJohnnyBrown/empty-course-error

show a friendly message for an empty course rather than an error page

AUTHORS

@@ -151,3 +151,4 @@ Steven Burch <stv@stanford.edu>
 Waqas Khalid <wkhalid@edx.org>
 Muhammad Ammar <mammar@edx.org>
 Abdallah Nassif <abdoosh00@gmail.com>
+Johnny Brown <johnnybrown7@gmail.com>

common/lib/xmodule/xmodule/modulestore/tests/test_split_modulestore.py

@@ -506,6 +506,42 @@ class SplitModuleTest(unittest.TestCase):
                 return element
 
 
+class TestHasChildrenAtDepth(SplitModuleTest):
+    """Test the has_children_at_depth method of XModuleMixin. """
+
+    def test_has_children_at_depth(self):
+        course_locator = CourseLocator(
+            org='testx', offering='GreekHero', branch='draft'
+        )
+        block_locator = BlockUsageLocator(
+            course_locator, 'course', 'head12345'
+        )
+        block = modulestore().get_item(block_locator)
+
+        self.assertRaises(
+            ValueError, block.has_children_at_depth, -1,
+        )
+        self.assertTrue(block.has_children_at_depth(0))
+        self.assertTrue(block.has_children_at_depth(1))
+        self.assertFalse(block.has_children_at_depth(2))
+
+        ch1 = modulestore().get_item(
+            BlockUsageLocator(course_locator, 'chapter', block_id='chapter1')
+        )
+        self.assertFalse(ch1.has_children_at_depth(0))
+
+        ch2 = modulestore().get_item(
+            BlockUsageLocator(course_locator, 'chapter', block_id='chapter2')
+        )
+        self.assertFalse(ch2.has_children_at_depth(0))
+
+        ch3 = modulestore().get_item(
+            BlockUsageLocator(course_locator, 'chapter', block_id='chapter3')
+        )
+        self.assertTrue(ch3.has_children_at_depth(0))
+        self.assertFalse(ch3.has_children_at_depth(1))
+
+
 class SplitModuleCourseTests(SplitModuleTest):
     '''
     Course CRUD operation tests

common/lib/xmodule/xmodule/x_module.py

@@ -236,6 +236,29 @@ class XModuleMixin(XBlockMixin):
                 result[field.name] = field.read_json(self)
         return result
 
+    def has_children_at_depth(self, depth):
+        """
+        Returns true if self has children at the given depth. depth==0 returns
+        false if self is a leaf, true otherwise.
+
+                           SELF
+                            |
+                     [child at depth 0]
+                     /           \
+                 [depth 1]    [depth 1]
+                 /       \
+           [depth 2]   [depth 2]
+
+        So the example above would return True for `has_children_at_depth(2)`, and False
+        for depth > 2
+        """
+        if depth < 0:
+            raise ValueError("negative depth argument is invalid")
+        elif depth == 0:
+            return bool(self.get_children())
+        else:
+            return any(child.has_children_at_depth(depth - 1) for child in self.get_children())
+
     def get_content_titles(self):
         """
         Returns list of content titles for all of self's children.

lms/djangoapps/courseware/tests/test_navigation.py

@@ -13,6 +13,7 @@ from xmodule.modulestore.tests.django_utils import ModuleStoreTestCase
 
 from courseware.tests.helpers import LoginEnrollmentTestCase, check_for_get_code
 from courseware.tests.modulestore_config import TEST_DATA_MIXED_MODULESTORE
+from courseware.tests.factories import GlobalStaffFactory
 
 
 @override_settings(MODULESTORE=TEST_DATA_MIXED_MODULESTORE)
@@ -35,6 +36,8 @@ class TestNavigation(ModuleStoreTestCase, LoginEnrollmentTestCase):
                                            display_name='Welcome')
         self.section9 = ItemFactory.create(parent_location=self.chapter9.location,
                                            display_name='factory_section')
+        self.unit0 = ItemFactory.create(parent_location=self.section0.location,
+                                        display_name='New Unit')
 
         # Create student accounts and activate them.
         for i in range(len(self.STUDENT_INFO)):
@@ -43,6 +46,8 @@ class TestNavigation(ModuleStoreTestCase, LoginEnrollmentTestCase):
             self.create_account(username, email, password)
             self.activate_user(email)
 
+        self.staff_user = GlobalStaffFactory()
+
     @override_settings(SESSION_INACTIVITY_TIMEOUT_IN_SECONDS=1)
     def test_inactive_session_timeout(self):
         """
@@ -135,3 +140,55 @@ class TestNavigation(ModuleStoreTestCase, LoginEnrollmentTestCase):
         self.assertRedirects(resp, reverse('courseware_chapter',
                                            kwargs={'course_id': self.course.id.to_deprecated_string(),
                                                    'chapter': 'factory_chapter'}))
+
+    def test_incomplete_course(self):
+        email = self.staff_user.email
+        password = "test"
+        self.login(email, password)
+        self.enroll(self.test_course, True)
+
+        test_course_id = self.test_course.id.to_deprecated_string()
+
+        check_for_get_code(
+            self, 200,
+            reverse(
+                'courseware',
+                kwargs={'course_id': test_course_id}
+            )
+        )
+
+        section = ItemFactory.create(
+            parent_location=self.test_course.location,
+            display_name='New Section'
+        )
+        check_for_get_code(
+            self, 200,
+            reverse(
+                'courseware',
+                kwargs={'course_id': test_course_id}
+            )
+        )
+
+        subsection = ItemFactory.create(
+            parent_location=section.location,
+            display_name='New Subsection'
+        )
+        check_for_get_code(
+            self, 200,
+            reverse(
+                'courseware',
+                kwargs={'course_id': test_course_id}
+            )
+        )
+
+        ItemFactory.create(
+            parent_location=subsection.location,
+            display_name='New Unit'
+        )
+        check_for_get_code(
+            self, 302,
+            reverse(
+                'courseware',
+                kwargs={'course_id': test_course_id}
+            )
+        )

lms/djangoapps/courseware/tests/test_view_authentication.py

@@ -112,23 +112,39 @@ class TestViewAuth(ModuleStoreTestCase, LoginEnrollmentTestCase):
     def setUp(self):
 
         self.course = CourseFactory.create(number='999', display_name='Robot_Super_Course')
-        self.overview_chapter = ItemFactory.create(display_name='Overview')
         self.courseware_chapter = ItemFactory.create(display_name='courseware')
+        self.overview_chapter = ItemFactory.create(
+            parent_location=self.course.location,
+            display_name='Super Overview'
+        )
+        self.welcome_section = ItemFactory.create(
+            parent_location=self.overview_chapter.location,
+            display_name='Super Welcome'
+        )
+        self.welcome_unit = ItemFactory.create(
+            parent_location=self.welcome_section.location,
+            display_name='Super Unit'
+        )
         self.course = modulestore().get_course(self.course.id)
 
         self.test_course = CourseFactory.create(number='666', display_name='Robot_Sub_Course')
         self.other_org_course = CourseFactory.create(org='Other_Org_Course')
         self.sub_courseware_chapter = ItemFactory.create(
-            parent_location=self.test_course.location, display_name='courseware'
+            parent_location=self.test_course.location,
+            display_name='courseware'
         )
         self.sub_overview_chapter = ItemFactory.create(
             parent_location=self.sub_courseware_chapter.location,
             display_name='Overview'
         )
-        self.welcome_section = ItemFactory.create(
-            parent_location=self.overview_chapter.location,
+        self.sub_welcome_section = ItemFactory.create(
+            parent_location=self.sub_overview_chapter.location,
             display_name='Welcome'
         )
+        self.sub_welcome_unit = ItemFactory.create(
+            parent_location=self.sub_welcome_section.location,
+            display_name='New Unit'
+        )
         self.test_course = modulestore().get_course(self.test_course.id)
 
         self.global_staff_user = GlobalStaffFactory()
@@ -151,9 +167,13 @@ class TestViewAuth(ModuleStoreTestCase, LoginEnrollmentTestCase):
         self.login(self.unenrolled_user)
         response = self.client.get(reverse('courseware',
                                            kwargs={'course_id': self.course.id.to_deprecated_string()}))
-        self.assertRedirects(response,
-                             reverse('about_course',
-                                     args=[self.course.id.to_deprecated_string()]))
+        self.assertRedirects(
+            response,
+            reverse(
+                'about_course',
+                args=[self.course.id.to_deprecated_string()]
+            )
+        )
 
     def test_redirection_enrolled(self):
         """
@@ -162,14 +182,22 @@ class TestViewAuth(ModuleStoreTestCase, LoginEnrollmentTestCase):
         """
         self.login(self.enrolled_user)
 
-        response = self.client.get(reverse('courseware',
-                                           kwargs={'course_id': self.course.id.to_deprecated_string()}))
+        response = self.client.get(
+            reverse(
+                'courseware',
+                kwargs={'course_id': self.course.id.to_deprecated_string()}
+            )
+        )
 
-        self.assertRedirects(response,
-                             reverse('courseware_section',
+        self.assertRedirects(
+            response,
+            reverse(
+                'courseware_section',
                 kwargs={'course_id': self.course.id.to_deprecated_string(),
-                                             'chapter': 'Overview',
-                                             'section': 'Welcome'}))
+                        'chapter': self.overview_chapter.url_name,
+                        'section': self.welcome_section.url_name}
+            )
+        )
 
     def test_instructor_page_access_nonstaff(self):
         """
@@ -314,6 +342,7 @@ class TestViewAuth(ModuleStoreTestCase, LoginEnrollmentTestCase):
         """
         now = datetime.datetime.now(pytz.UTC)
         tomorrow = now + datetime.timedelta(days=1)
+
         self.course.start = tomorrow
         self.test_course.start = tomorrow
         self.course = self.update_course(self.course)

lms/djangoapps/courseware/tests/test_views.py

@@ -145,7 +145,7 @@ class ViewsTestCase(TestCase):
         mock_module.position = 3
         mock_module.get_display_items.return_value = []
         self.assertRaises(Http404, views.redirect_to_course_position,
-                          mock_module)
+                          mock_module, views.CONTENT_DEPTH)
 
     def test_registered_for_course(self):
         self.assertFalse(views.registered_for_course('Basketweaving', None))

lms/djangoapps/courseware/views.py

@@ -52,6 +52,7 @@ log = logging.getLogger("edx.courseware")
 
 template_imports = {'urllib': urllib}
 
+CONTENT_DEPTH = 2
 
 def user_groups(user):
     """
@@ -113,17 +114,36 @@ def render_accordion(request, course, chapter, section, field_data_cache):
     return render_to_string('courseware/accordion.html', context)
 
 
-def get_current_child(xmodule):
+def get_current_child(xmodule, min_depth=None):
     """
     Get the xmodule.position's display item of an xmodule that has a position and
-    children.  If xmodule has no position or is out of bounds, return the first child.
+    children.  If xmodule has no position or is out of bounds, return the first
+    child with children extending down to content_depth.
+
+    For example, if chapter_one has no position set, with two child sections,
+    section-A having no children and section-B having a discussion unit,
+    `get_current_child(chapter, min_depth=1)`  will return section-B.
+
     Returns None only if there are no children at all.
     """
+    def _get_default_child_module(child_modules):
+        """Returns the first child of xmodule, subject to min_depth."""
+        if not child_modules:
+            default_child = None
+        elif not min_depth > 0:
+            default_child = child_modules[0]
+        else:
+            content_children = [child for child in child_modules if
+                                child.has_children_at_depth(min_depth - 1)]
+            default_child = content_children[0] if content_children else None
+
+        return default_child
+
     if not hasattr(xmodule, 'position'):
         return None
 
     if xmodule.position is None:
-        pos = 0
+        return _get_default_child_module(xmodule.get_display_items())
     else:
         # position is 1-indexed.
         pos = xmodule.position - 1
@@ -132,14 +152,15 @@ def get_current_child(xmodule):
     if 0 <= pos < len(children):
         child = children[pos]
     elif len(children) > 0:
-        # Something is wrong.  Default to first child
-        child = children[0]
+        # module has a set position, but the position is out of range.
+        # return default child.
+        child = _get_default_child_module(children)
     else:
         child = None
     return child
 
 
-def redirect_to_course_position(course_module):
+def redirect_to_course_position(course_module, content_depth):
     """
     Return a redirect to the user's current place in the course.
 
@@ -153,7 +174,7 @@ def redirect_to_course_position(course_module):
 
     """
     urlargs = {'course_id': course_module.id.to_deprecated_string()}
-    chapter = get_current_child(course_module)
+    chapter = get_current_child(course_module, min_depth=content_depth)
     if chapter is None:
         # oops.  Something bad has happened.
         raise Http404("No chapter found when loading current position in course")
@@ -163,7 +184,7 @@ def redirect_to_course_position(course_module):
         return redirect(reverse('courseware_chapter', kwargs=urlargs))
 
     # Relying on default of returning first child
-    section = get_current_child(chapter)
+    section = get_current_child(chapter, min_depth=content_depth - 1)
     if section is None:
         raise Http404("No section found when loading current position in course")
 
@@ -266,9 +287,6 @@ def index(request, course_id, chapter=None, section=None,
 
         studio_url = get_studio_url(course_key, 'course')
 
-        if chapter is None:
-            return redirect_to_course_position(course_module)
-
         context = {
             'csrf': csrf(request)['csrf_token'],
             'accordion': render_accordion(request, course, chapter, section, field_data_cache),
@@ -283,6 +301,15 @@ def index(request, course_id, chapter=None, section=None,
             'reverifications': fetch_reverify_banner_info(request, course_key),
         }
 
+        has_content = course.has_children_at_depth(CONTENT_DEPTH)
+        if not has_content:
+            # Show empty courseware for a course with no units
+            return render_to_response('courseware/courseware.html', context)
+        elif chapter is None:
+            # passing CONTENT_DEPTH avoids returning 404 for a course with an
+            # empty first section and a second section with content
+            return redirect_to_course_position(course_module, CONTENT_DEPTH)
+
         # Only show the chat if it's enabled by the course and in the
         # settings.
         show_chat = course.show_chat and settings.FEATURES['ENABLE_CHAT']

lms/templates/courseware/courseware.html

@@ -195,7 +195,11 @@ ${fragment.foot_html()}
 
       <div id="accordion" style="display: none">
         <nav aria-label="${_('Course Navigation')}">
+          % if accordion.strip():
             ${accordion}
+          % else:
+            <div class="chapter">${_("No content has been added to this course")}</div>
+          % endif
         </nav>
       </div>
     </div>