Merge pull request #14962 from edx/robrap/LEARNER-646

LEARNER-646: Resume Course button should say "Start Course" if the user has not yet started

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

@@ -23,7 +23,6 @@ from xmodule.modulestore.django import modulestore, clear_existing_modulestores,
 from xmodule.modulestore.tests.mongo_connection import MONGO_PORT_NUM, MONGO_HOST
 from xmodule.modulestore.tests.factories import XMODULE_FACTORY_LOCK
 
-from openedx.core.djangoapps.bookmarks.signals import trigger_update_xblocks_cache_task
 from openedx.core.djangolib.testing.utils import CacheIsolationMixin, CacheIsolationTestCase
 
 

lms/djangoapps/course_blocks/transformers/library_content.py

@@ -12,6 +12,8 @@ from xmodule.modulestore.django import modulestore
 from eventtracking import tracker
 from track import contexts
 
+from ..utils import get_student_module_as_dict
+
 
 class ContentLibraryTransformer(FilteringTransformerMixin, BlockStructureTransformer):
     """
@@ -78,12 +80,7 @@ class ContentLibraryTransformer(FilteringTransformerMixin, BlockStructureTransfo
                 max_count = block_structure.get_xblock_field(block_key, 'max_count')
 
                 # Retrieve "selected" json from LMS MySQL database.
-                module = self._get_student_module(usage_info.user, usage_info.course_key, block_key)
-                if module:
-                    state_dict = json.loads(module.state)
-                else:
-                    state_dict = {}
-
+                state_dict = get_student_module_as_dict(usage_info.user, usage_info.course_key, block_key)
                 for selected_block in state_dict.get('selected', []):
                     # Add all selected entries for this user for this
                     # library module to the selected list.
@@ -135,28 +132,6 @@ class ContentLibraryTransformer(FilteringTransformerMixin, BlockStructureTransfo
 
         return [block_structure.create_removal_filter(check_child_removal)]
 
-    @classmethod
-    def _get_student_module(cls, user, course_key, block_key):
-        """
-        Get the student module for the given user for the given block.
-
-        Arguments:
-            user (User)
-            course_key (CourseLocator)
-            block_key (BlockUsageLocator)
-
-        Returns:
-            StudentModule if exists, or None.
-        """
-        try:
-            return StudentModule.objects.get(
-                student=user,
-                course_id=course_key,
-                module_state_key=block_key,
-            )
-        except StudentModule.DoesNotExist:
-            return None
-
     def _publish_events(self, block_structure, location, previous_count, max_count, block_keys, user_id):
         """
         Helper method to publish events for analytics purposes

lms/djangoapps/course_blocks/utils.py

+"""
+Common utilities for use along with the course blocks.
+"""
+import json
+from courseware.models import StudentModule
+
+
+def get_student_module_as_dict(user, course_key, block_key):
+    """
+    Get the student module as a dict for the given user for the given block.
+
+    Arguments:
+        user (User)
+        course_key (CourseLocator)
+        block_key (BlockUsageLocator)
+
+    Returns:
+        StudentModule as a (possibly empty) dict.
+    """
+    try:
+        student_module = StudentModule.objects.get(
+            student=user,
+            course_id=course_key,
+            module_state_key=block_key,
+        )
+    except StudentModule.DoesNotExist:
+        student_module = None
+
+    if student_module:
+        return json.loads(student_module.state)
+    else:
+        return {}

lms/djangoapps/courseware/courses.py

@@ -518,28 +518,3 @@ def get_current_child(xmodule, min_depth=None, requested_child=None):
                 child = _get_default_child_module(children)
 
     return child
-
-
-def get_last_accessed_courseware(course, request, user):
-    """
-    Returns a tuple containing the courseware module (URL, id) that the user last accessed,
-    or (None, None) if it cannot be found.
-    """
-    # TODO: convert this method to use the Course Blocks API
-    field_data_cache = FieldDataCache.cache_for_descriptor_descendents(
-        course.id, request.user, course, depth=2
-    )
-    course_module = get_module_for_descriptor(
-        user, request, course, field_data_cache, course.id, course=course
-    )
-    chapter_module = get_current_child(course_module)
-    if chapter_module is not None:
-        section_module = get_current_child(chapter_module)
-        if section_module is not None:
-            url = reverse('courseware_section', kwargs={
-                'course_id': unicode(course.id),
-                'chapter': chapter_module.url_name,
-                'section': section_module.url_name
-            })
-            return (url, section_module.url_name)
-    return (None, None)

lms/djangoapps/courseware/views/views.py

@@ -66,7 +66,7 @@ from courseware.courses import (
     get_course_by_id,
     get_course_overview_with_access,
     get_course_with_access,
-    get_last_accessed_courseware,
+    get_current_child,
     get_permission_for_course_about,
     get_studio_url,
     sort_by_announcement,
@@ -97,7 +97,6 @@ from openedx.features.course_experience import (
     UNIFIED_COURSE_VIEW_FLAG,
 )
 from openedx.features.enterprise_support.api import data_sharing_consent_required
-from shoppingcart.models import CourseRegistrationCode
 from shoppingcart.utils import is_shopping_cart_enabled
 from student.models import UserTestGroup, CourseEnrollment
 from survey.utils import must_answer_survey
@@ -249,6 +248,28 @@ def course_info(request, course_id):
 
     Assumes the course_id is in a valid format.
     """
+    def get_last_accessed_courseware(course, request, user):
+        """
+        Returns the courseware module URL that the user last accessed, or None if it cannot be found.
+        """
+        field_data_cache = FieldDataCache.cache_for_descriptor_descendents(
+            course.id, request.user, course, depth=2
+        )
+        course_module = get_module_for_descriptor(
+            user, request, course, field_data_cache, course.id, course=course
+        )
+        chapter_module = get_current_child(course_module)
+        if chapter_module is not None:
+            section_module = get_current_child(chapter_module)
+            if section_module is not None:
+                url = reverse('courseware_section', kwargs={
+                    'course_id': unicode(course.id),
+                    'chapter': chapter_module.url_name,
+                    'section': section_module.url_name
+                })
+                return url
+        return None
+
     # If the unified course experience is enabled, redirect to the "Course" tab
     if waffle.flag_is_active(request, UNIFIED_COURSE_EXPERIENCE_FLAG):
         return redirect(reverse(course_home_url_name(request), args=[course_id]))
@@ -343,7 +364,7 @@ def course_info(request, course_id):
         # Get the URL of the user's last position in order to display the 'where you were last' message
         context['last_accessed_courseware_url'] = None
         if SelfPacedConfiguration.current().enable_course_home_improvements:
-            context['last_accessed_courseware_url'], _ = get_last_accessed_courseware(course, request, user)
+            context['last_accessed_courseware_url'] = get_last_accessed_courseware(course, request, user)
 
         now = datetime.now(UTC())
         effective_start = _adjust_start_date_for_beta_testers(user, course, course_key)

openedx/features/course_experience/templates/course_experience/course-home-fragment.html

@@ -50,17 +50,19 @@ from openedx.features.course_experience import UNIFIED_COURSE_EXPERIENCE_FLAG
             % endif
             <div class="form-actions">
                 % if not waffle.flag_is_active(request, UNIFIED_COURSE_EXPERIENCE_FLAG):
-                    <a class="btn action-show-bookmarks" href="${reverse('openedx.course_bookmarks.home', args=[course.id])}">
+                    <a class="btn action-show-bookmarks" href="${reverse('openedx.course_bookmarks.home', args=[course_key])}">
                         ${_("Bookmarks")}
                     </a>
                 % endif
-                <a class="btn btn-brand action-resume-course" href="${reverse('courseware', kwargs={'course_id': unicode(course.id.to_deprecated_string())})}">
-                    % if has_visited_course:
-                        ${_("Resume Course")}
-                    % else:
-                        ${_("Start Course")}
-                    % endif
-                </a>
+                % if resume_course_url:
+                    <a class="btn btn-brand action-resume-course" href="${resume_course_url}">
+                        % if has_visited_course:
+                            ${_("Resume Course")}
+                        % else:
+                            ${_("Start Course")}
+                        % endif
+                    </a>
+                % endif
             </div>
         </div>
     </header>
@@ -75,7 +77,7 @@ from openedx.features.course_experience import UNIFIED_COURSE_EXPERIENCE_FLAG
                         <h3 class="hd-6">${_("Course Tools")}</h3>
                         <ul class="list-unstyled">
                             <li>
-                                <a class="action-show-bookmarks" href="${reverse('openedx.course_bookmarks.home', args=[course.id])}">
+                                <a class="action-show-bookmarks" href="${reverse('openedx.course_bookmarks.home', args=[course_key])}">
                                     <span class="icon fa fa-bookmark" aria-hidden="true"></span>
                                     ${_("Bookmarks")}
                                 </a>

openedx/features/course_experience/templates/course_experience/course-outline-fragment.html

@@ -27,9 +27,9 @@ from openedx.core.djangolib.markup import HTML, Text
                         <h3>${ section['display_name'] }</h3>
                     </div>
                     <ol class="outline-item focusable" role="group" tabindex="0">
-                        % for subsection in section.get('children') or []:
+                        % for subsection in section.get('children', []):
                             <li
-                                class="subsection ${ 'current' if subsection['current'] else '' }"
+                                class="subsection ${ 'current' if subsection['last_accessed'] else '' }"
                                 role="treeitem"
                                 tabindex="-1"
                                 aria-expanded="true"
@@ -106,7 +106,7 @@ from openedx.core.djangolib.markup import HTML, Text
                                     </div> <!-- /subsection-text -->
                                     <div class="subsection-actions">
                                         ## Resume button (if last visited section)
-                                        % if subsection['current']:
+                                        % if subsection['last_accessed']:
                                             <span class="sr-only">${ _("This is your last visited course section.") }</span>
                                             <span class="resume-right">
                                                 <b>${ _("Resume Course") }</b>

openedx/features/course_experience/tests/views/test_course_outline.py

@@ -3,10 +3,10 @@ Tests for the Course Outline view and supporting views.
 """
 import datetime
 import ddt
-from mock import patch
 import json
 
 from django.core.urlresolvers import reverse
+from pyquery import PyQuery as pq
 
 from courseware.tests.factories import StaffFactory
 from student.models import CourseEnrollment
@@ -37,9 +37,10 @@ class TestCourseOutlinePage(SharedModuleStoreTestCase):
             with cls.store.bulk_operations(course.id):
                 chapter = ItemFactory.create(category='chapter', parent_location=course.location)
                 section = ItemFactory.create(category='sequential', parent_location=chapter.location)
-                ItemFactory.create(category='vertical', parent_location=section.location)
-                course.last_accessed = section.url_name
-
+                vertical = ItemFactory.create(category='vertical', parent_location=section.location)
+            course.children = [chapter]
+            chapter.children = [section]
+            section.children = [vertical]
             cls.courses.append(course)
 
             course = CourseFactory.create()
@@ -47,10 +48,12 @@ class TestCourseOutlinePage(SharedModuleStoreTestCase):
                 chapter = ItemFactory.create(category='chapter', parent_location=course.location)
                 section = ItemFactory.create(category='sequential', parent_location=chapter.location)
                 section2 = ItemFactory.create(category='sequential', parent_location=chapter.location)
-                ItemFactory.create(category='vertical', parent_location=section.location)
-                ItemFactory.create(category='vertical', parent_location=section2.location)
-                course.last_accessed = None
-
+                vertical = ItemFactory.create(category='vertical', parent_location=section.location)
+                vertical2 = ItemFactory.create(category='vertical', parent_location=section2.location)
+            course.children = [chapter]
+            chapter.children = [section, section2]
+            section.children = [vertical]
+            section2.children = [vertical2]
             cls.courses.append(course)
 
             course = CourseFactory.create()
@@ -63,8 +66,10 @@ class TestCourseOutlinePage(SharedModuleStoreTestCase):
                     graded=True,
                     format='Homework',
                 )
-                ItemFactory.create(category='vertical', parent_location=section.location)
-                course.last_accessed = section.url_name
+                vertical = ItemFactory.create(category='vertical', parent_location=section.location)
+            course.children = [chapter]
+            chapter.children = [section]
+            section.children = [vertical]
             cls.courses.append(course)
 
     @classmethod
@@ -81,26 +86,79 @@ class TestCourseOutlinePage(SharedModuleStoreTestCase):
         super(TestCourseOutlinePage, self).setUp()
         self.client.login(username=self.user.username, password=TEST_PASSWORD)
 
-    @patch('openedx.features.course_experience.views.course_outline.get_last_accessed_courseware')
-    def test_render(self, patched_get_last_accessed):
+    def test_outline_details(self):
         for course in self.courses:
-            patched_get_last_accessed.return_value = (None, course.last_accessed)
+
             url = course_home_url(course)
             response = self.client.get(url)
             self.assertEqual(response.status_code, 200)
             response_content = response.content.decode("utf-8")
 
-            self.assertIn('Resume Course', response_content)
+            self.assertTrue(course.children)
             for chapter in course.children:
                 self.assertIn(chapter.display_name, response_content)
+                self.assertTrue(chapter.children)
                 for section in chapter.children:
                     self.assertIn(section.display_name, response_content)
                     if section.graded:
-                        self.assertIn(section.due, response_content)
+                        self.assertIn(section.due.strftime('%Y-%m-%d %H:%M:%S'), response_content)
                         self.assertIn(section.format, response_content)
+                    self.assertTrue(section.children)
                     for vertical in section.children:
                         self.assertNotIn(vertical.display_name, response_content)
 
+    def test_start_course(self):
+        """
+        Tests that the start course button appears when the course has never been accessed.
+
+        Technically, this is a course home test, and not a course outline test, but checking the counts of
+        start/resume course should be done together to not get a false positive.
+
+        """
+        course = self.courses[0]
+
+        response = self.client.get(course_home_url(course))
+        self.assertEqual(response.status_code, 200)
+
+        self.assertContains(response, 'Start Course', count=1)
+        self.assertContains(response, 'Resume Course', count=0)
+
+        content = pq(response.content)
+        self.assertTrue(content('.action-resume-course').attr('href').endswith('/course/' + course.url_name))
+
+    def test_resume_course(self):
+        """
+        Tests that two resume course buttons appear when the course has been accessed.
+
+        Technically, this is a mix of a course home and course outline test, but checking the counts of start/resume
+        course should be done together to not get a false positive.
+
+        """
+        course = self.courses[0]
+
+        # first navigate to a section to make it the last accessed
+        chapter = course.children[0]
+        section = chapter.children[0]
+        last_accessed_url = reverse(
+            'courseware_section',
+            kwargs={
+                'course_id': course.id.to_deprecated_string(),
+                'chapter': chapter.url_name,
+                'section': section.url_name,
+            }
+        )
+        self.assertEqual(200, self.client.get(last_accessed_url).status_code)
+
+        # check resume course buttons
+        response = self.client.get(course_home_url(course))
+        self.assertEqual(response.status_code, 200)
+
+        self.assertContains(response, 'Start Course', count=0)
+        self.assertContains(response, 'Resume Course', count=2)
+
+        content = pq(response.content)
+        self.assertTrue(content('.action-resume-course').attr('href').endswith('/sequential/' + section.url_name))
+
 
 class TestCourseOutlinePreview(SharedModuleStoreTestCase):
     """

openedx/features/course_experience/utils.py

+"""
+Common utilities for the course experience, including course outline.
+"""
+from lms.djangoapps.course_api.blocks.api import get_blocks
+from lms.djangoapps.course_blocks.utils import get_student_module_as_dict
+from opaque_keys.edx.keys import CourseKey
+from openedx.core.lib.cache_utils import memoized
+from xmodule.modulestore.django import modulestore
+
+
+@memoized
+def get_course_outline_block_tree(request, course_id):
+    """
+    Returns the root block of the course outline, with children as blocks.
+    """
+
+    def populate_children(block, all_blocks):
+        """
+        Replace each child id with the full block for the child.
+
+        Given a block, replaces each id in its children array with the full
+        representation of that child, which will be looked up by id in the
+        passed all_blocks dict. Recursively do the same replacement for children
+        of those children.
+        """
+        children = block.get('children', [])
+
+        for i in range(len(children)):
+            child_id = block['children'][i]
+            child_detail = populate_children(all_blocks[child_id], all_blocks)
+            block['children'][i] = child_detail
+
+        return block
+
+    def set_lasted_accessed_default(block):
+        """
+        Set default of False for last_accessed on all blocks.
+        """
+        block['last_accessed'] = False
+        for child in block.get('children', []):
+            set_lasted_accessed_default(child)
+
+    def mark_lasted_accessed(user, course_key, block):
+        """
+        Recursively marks the branch to the last accessed block.
+        """
+        block_key = block.serializer.instance
+        student_module_dict = get_student_module_as_dict(user, course_key, block_key)
+        last_accessed_child_position = student_module_dict.get('position')
+        if last_accessed_child_position and block.get('children'):
+            block['last_accessed'] = True
+            if len(block['children']) <= last_accessed_child_position:
+                last_accessed_child_block = block['children'][last_accessed_child_position - 1]
+                last_accessed_child_block['last_accessed'] = True
+                mark_lasted_accessed(user, course_key, last_accessed_child_block)
+            else:
+                # We should be using an id in place of position for last accessed. However, while using position, if
+                # the child block is no longer accessible we'll use the last child.
+                block['children'][-1]['last_accessed'] = True
+
+    course_key = CourseKey.from_string(course_id)
+    course_usage_key = modulestore().make_course_usage_key(course_key)
+
+    all_blocks = get_blocks(
+        request,
+        course_usage_key,
+        user=request.user,
+        nav_depth=3,
+        requested_fields=['children', 'display_name', 'type', 'due', 'graded', 'special_exam_info', 'format'],
+        block_types_filter=['course', 'chapter', 'sequential']
+    )
+
+    course_outline_root_block = all_blocks['blocks'][all_blocks['root']]
+    populate_children(course_outline_root_block, all_blocks['blocks'])
+    set_lasted_accessed_default(course_outline_root_block)
+    mark_lasted_accessed(request.user, course_key, course_outline_root_block)
+
+    return course_outline_root_block

openedx/features/course_experience/views/course_home.py

@@ -9,14 +9,15 @@ from django.utils.decorators import method_decorator
 from django.views.decorators.cache import cache_control
 from django.views.decorators.csrf import ensure_csrf_cookie
 
-from courseware.courses import get_course_info_section, get_course_with_access, get_last_accessed_courseware
+from courseware.courses import get_course_info_section, get_course_with_access
 from lms.djangoapps.courseware.views.views import CourseTabView
 from opaque_keys.edx.keys import CourseKey
 from openedx.core.djangoapps.plugin_api.views import EdxFragmentView
 from util.views import ensure_valid_course_key
 from web_fragments.fragment import Fragment
 
-from course_outline import CourseOutlineFragmentView
+from .course_outline import CourseOutlineFragmentView
+from ..utils import get_course_outline_block_tree
 
 
 class CourseHomeView(CourseTabView):
@@ -43,29 +44,67 @@ class CourseHomeFragmentView(EdxFragmentView):
     """
     A fragment to render the home page for a course.
     """
+
+    def _get_resume_course_info(self, request, course_id):
+        """
+        Returns information relevant to resume course functionality.
+
+        Returns a tuple: (has_visited_course, resume_course_url)
+            has_visited_course: True if the user has ever visted the course, False otherwise.
+            resume_course_url: The URL of the last accessed block if the user has visited the course,
+                otherwise the URL of the course root.
+
+        """
+
+        def get_last_accessed_block(block):
+            """
+            Gets the deepest block marked as 'last_accessed'.
+            """
+            if not block['last_accessed']:
+                return None
+            if not block.get('children'):
+                return block
+            for child in block['children']:
+                last_accessed_block = get_last_accessed_block(child)
+                if last_accessed_block:
+                    return last_accessed_block
+            return block
+
+        course_outline_root_block = get_course_outline_block_tree(request, course_id)
+        last_accessed_block = get_last_accessed_block(course_outline_root_block)
+        has_visited_course = bool(last_accessed_block)
+        if last_accessed_block:
+            resume_course_url = last_accessed_block['lms_web_url']
+        else:
+            resume_course_url = course_outline_root_block['lms_web_url']
+
+        return (has_visited_course, resume_course_url)
+
     def render_to_fragment(self, request, course_id=None, **kwargs):
         """
         Renders the course's home page as a fragment.
         """
         course_key = CourseKey.from_string(course_id)
-        course = get_course_with_access(request.user, 'load', course_key, check_if_enrolled=True)
 
         # Render the outline as a fragment
         outline_fragment = CourseOutlineFragmentView().render_to_fragment(request, course_id=course_id, **kwargs)
 
-        # Get the last accessed courseware
-        last_accessed_url, __ = get_last_accessed_courseware(course, request, request.user)
+        # Get resume course information
+        has_visited_course, resume_course_url = self._get_resume_course_info(request, course_id)
 
         # Get the handouts
+        # TODO: Use get_course_overview_with_access and blocks api
+        course = get_course_with_access(request.user, 'load', course_key, check_if_enrolled=True)
         handouts_html = get_course_info_section(request, request.user, course, 'handouts')
 
         # Render the course home fragment
         context = {
             'csrf': csrf(request)['csrf_token'],
-            'course': course,
+            'course_key': course_key,
             'outline_fragment': outline_fragment,
             'handouts_html': handouts_html,
-            'has_visited_course': last_accessed_url is not None,
+            'has_visited_course': has_visited_course,
+            'resume_course_url': resume_course_url,
             'disable_courseware_js': True,
             'uses_pattern_library': True,
         }

openedx/features/course_experience/views/course_outline.py

@@ -5,12 +5,12 @@ Views to show a course outline.
 from django.core.context_processors import csrf
 from django.template.loader import render_to_string
 
-from courseware.courses import get_course_with_access, get_last_accessed_courseware
-from lms.djangoapps.course_api.blocks.api import get_blocks
+from courseware.courses import get_course_overview_with_access
 from opaque_keys.edx.keys import CourseKey
 from openedx.core.djangoapps.plugin_api.views import EdxFragmentView
 from web_fragments.fragment import Fragment
-from xmodule.modulestore.django import modulestore
+
+from ..utils import get_course_outline_block_tree
 
 
 class CourseOutlineFragmentView(EdxFragmentView):
@@ -18,47 +18,19 @@ class CourseOutlineFragmentView(EdxFragmentView):
     Course outline fragment to be shown in the unified course view.
     """
 
-    def populate_children(self, block, all_blocks, course_position):
-        """
-        For a passed block, replace each id in its children array with the full representation of that child,
-        which will be looked up by id in the passed all_blocks dict.
-        Recursively do the same replacement for children of those children.
-        """
-        children = block.get('children') or []
-
-        for i in range(len(children)):
-            child_id = block['children'][i]
-            child_detail = self.populate_children(all_blocks[child_id], all_blocks, course_position)
-
-            block['children'][i] = child_detail
-            block['children'][i]['current'] = course_position == child_detail['block_id']
-
-        return block
-
     def render_to_fragment(self, request, course_id=None, page_context=None, **kwargs):
         """
         Renders the course outline as a fragment.
         """
         course_key = CourseKey.from_string(course_id)
-        course = get_course_with_access(request.user, 'load', course_key, check_if_enrolled=True)
-        _, course_position = get_last_accessed_courseware(course, request, request.user)
-        course_usage_key = modulestore().make_course_usage_key(course_key)
-        all_blocks = get_blocks(
-            request,
-            course_usage_key,
-            user=request.user,
-            nav_depth=3,
-            requested_fields=['children', 'display_name', 'type', 'due', 'graded', 'special_exam_info', 'format'],
-            block_types_filter=['course', 'chapter', 'sequential']
-        )
+        course_overview = get_course_overview_with_access(request.user, 'load', course_key, check_if_enrolled=True)
 
-        course_block_tree = all_blocks['blocks'][all_blocks['root']]  # Get the root of the block tree
+        course_block_tree = get_course_outline_block_tree(request, course_id)
 
         context = {
             'csrf': csrf(request)['csrf_token'],
-            'course': course,
-            # Recurse through the block tree, fleshing out each child object
-            'blocks': self.populate_children(course_block_tree, all_blocks['blocks'], course_position)
+            'course': course_overview,
+            'blocks': course_block_tree
         }
         html = render_to_string('course_experience/course-outline-fragment.html', context)
         return Fragment(html)