Merge pull request #6797 from edx/mobile/perf_improvements

Mobile performance improvements

common/lib/xmodule/xmodule/video_module/video_module.py

@@ -304,14 +304,7 @@ class VideoDescriptor(VideoFields, VideoTranscriptsMixin, VideoStudioViewHandler
             self._field_data.set_many(self, field_data)
             del self.data
 
-        editable_fields = super(VideoDescriptor, self).editable_metadata_fields
-
         self.source_visible = False
-        # Set download_video field to default value if its not explicitly set for backward compatibility.
-        download_video = editable_fields['download_video']
-        if not download_video['explicitly_set']:
-            self.download_video = self.download_video
-
         if self.source:
             # If `source` field value exist in the `html5_sources` field values,
             # then delete `source` field value and use value from `html5_sources` field.
@@ -320,14 +313,17 @@ class VideoDescriptor(VideoFields, VideoTranscriptsMixin, VideoStudioViewHandler
                 self.download_video = True
             else:  # Otherwise, `source` field value will be used.
                 self.source_visible = True
-                if not download_video['explicitly_set']:
+                if not self.fields['download_video'].is_set_on(self):
                     self.download_video = True
 
+        # Set download_video field to default value if its not explicitly set for backward compatibility.
+        if not self.fields['download_video'].is_set_on(self):
+            self.download_video = self.download_video
+
         # for backward compatibility.
         # If course was existed and was not re-imported by the moment of adding `download_track` field,
         # we should enable `download_track` if following is true:
-        download_track = editable_fields['download_track']
-        if not download_track['explicitly_set'] and self.track:
+        if not self.fields['download_track'].is_set_on(self) and self.track:
             self.download_track = True
 
     def editor_saved(self, user, old_metadata, old_content):

common/lib/xmodule/xmodule/x_module.py

@@ -403,7 +403,7 @@ class XModuleMixin(XBlockMixin):
         else:
             return [self.display_name_with_default]
 
-    def get_children(self):
+    def get_children(self, usage_key_filter=lambda location: True):
         """Returns a list of XBlock instances for the children of
         this module"""
 
@@ -413,6 +413,9 @@ class XModuleMixin(XBlockMixin):
         if getattr(self, '_child_instances', None) is None:
             self._child_instances = []  # pylint: disable=attribute-defined-outside-init
             for child_loc in self.children:
+                # Skip if it doesn't satisfy the filter function
+                if not usage_key_filter(child_loc):
+                    continue
                 try:
                     child = self.runtime.get_block(child_loc)
                     if child is None:

lms/djangoapps/courseware/tests/test_video_mongo.py

 # -*- coding: utf-8 -*-
 """Video xmodule tests in mongo."""
 import json
-import unittest
 from collections import OrderedDict
-from mock import patch, PropertyMock, MagicMock
+from mock import patch, MagicMock
 
 from django.conf import settings
 
-from xblock.fields import ScopeIds
-from xblock.field_data import DictFieldData
-
-from xmodule.video_module import create_youtube_string, VideoDescriptor
+from xmodule.video_module import create_youtube_string
 from xmodule.x_module import STUDENT_VIEW
-from xmodule.tests import get_test_descriptor_system
 from xmodule.tests.test_video import VideoDescriptorTestBase
 
-from edxval.api import (
-    ValVideoNotFoundError,
-    get_video_info,
-    create_profile,
-    create_video
-)
-import mock
+from edxval.api import create_profile, create_video
 
 from . import BaseTestXmodule
 from .test_video_xml import SOURCE_XML
@@ -417,7 +406,7 @@ class TestGetHtmlMethod(BaseTestXmodule):
         # it'll just fall back to the values in the VideoDescriptor.
         self.assertIn("example_source.mp4", self.item_descriptor.render(STUDENT_VIEW).content)
 
-    @mock.patch('edxval.api.get_video_info')
+    @patch('edxval.api.get_video_info')
     def test_get_html_with_mocked_edx_video_id(self, mock_get_video_info):
         mock_get_video_info.return_value = {
             'url': '/edxval/video/example',
@@ -798,81 +787,22 @@ class TestVideoDescriptorInitialization(BaseTestXmodule):
         self.assertFalse(self.item_descriptor.source_visible)
 
     def test_download_video_is_explicitly_set(self):
-        with patch(
-            'xmodule.editing_module.TabsEditingDescriptor.editable_metadata_fields',
-            new_callable=PropertyMock,
-            return_value={
-                'download_video': {
-                    'default_value': False,
-                    'explicitly_set': True,
-                    'display_name': 'Video Download Allowed',
-                    'help': 'Show a link beneath the video to allow students to download the video.',
-                    'type': 'Boolean',
-                    'value': False,
-                    'field_name': 'download_video',
-                    'options': [
-                        {'display_name': "True", "value": True},
-                        {'display_name': "False", "value": False}
-                    ],
-                },
-                'html5_sources': {
-                    'default_value': [],
-                    'explicitly_set': False,
-                    'display_name': 'Video Sources',
-                    'help': 'A list of filenames to be used with HTML5 video.',
-                    'type': 'List',
-                    'value': [u'http://youtu.be/3_yD_cEKoCk.mp4'],
-                    'field_name': 'html5_sources',
-                    'options': [],
-                },
-                'source': {
-                    'default_value': '',
-                    'explicitly_set': False,
-                    'display_name': 'Download Video',
-                    'help': 'The external URL to download the video.',
-                    'type': 'Generic',
-                    'value': u'http://example.org/video.mp4',
-                    'field_name': 'source',
-                    'options': [],
-                },
-                'track': {
-                    'default_value': '',
-                    'explicitly_set': False,
-                    'display_name': 'Download Transcript',
-                    'help': 'The external URL to download the timed transcript track.',
-                    'type': 'Generic',
-                    'value': u'http://some_track.srt',
-                    'field_name': 'track',
-                    'options': [],
-                },
-                'download_track': {
-                    'default_value': False,
-                    'explicitly_set': False,
-                    'display_name': 'Transcript Download Allowed',
-                    'help': 'Show a link beneath the video to allow students to download the transcript. Note: You must add a link to the HTML5 Transcript field above.',
-                    'type': 'Generic',
-                    'value': False,
-                    'field_name': 'download_track',
-                    'options': [],
-                },
-                'transcripts': {},
-                'handout': {},
-            }
-        ):
-            metadata = {
-                'track': u'http://some_track.srt',
-                'source': 'http://example.org/video.mp4',
-                'html5_sources': ['http://youtu.be/3_yD_cEKoCk.mp4'],
-            }
+        metadata = {
+            'track': u'http://some_track.srt',
+            'source': 'http://example.org/video.mp4',
+            'html5_sources': ['http://youtu.be/3_yD_cEKoCk.mp4'],
+            'download_video': False,
+        }
 
-            self.initialize_module(metadata=metadata)
+        self.initialize_module(metadata=metadata)
 
-            fields = self.item_descriptor.editable_metadata_fields
-            self.assertIn('source', fields)
+        fields = self.item_descriptor.editable_metadata_fields
+        self.assertIn('source', fields)
+        self.assertIn('download_video', fields)
 
-            self.assertFalse(self.item_descriptor.download_video)
-            self.assertTrue(self.item_descriptor.source_visible)
-            self.assertTrue(self.item_descriptor.download_track)
+        self.assertFalse(self.item_descriptor.download_video)
+        self.assertTrue(self.item_descriptor.source_visible)
+        self.assertTrue(self.item_descriptor.download_track)
 
     def test_source_is_empty(self):
         metadata = {

lms/djangoapps/mobile_api/users/tests.py

@@ -90,23 +90,32 @@ class CourseStatusAPITestCase(MobileAPITestCase):
     """
     REVERSE_INFO = {'name': 'user-course-status', 'params': ['username', 'course_id']}
 
-    def _setup_course_skeleton(self):
+    def setUp(self):
         """
         Creates a basic course structure for our course
         """
-        section = ItemFactory.create(
-            parent_location=self.course.location,
+        super(CourseStatusAPITestCase, self).setUp()
+
+        self.section = ItemFactory.create(
+            parent=self.course,
+            category='chapter',
+        )
+        self.sub_section = ItemFactory.create(
+            parent=self.section,
+            category='sequential',
         )
-        sub_section = ItemFactory.create(
-            parent_location=section.location,
+        self.unit = ItemFactory.create(
+            parent=self.sub_section,
+            category='vertical',
         )
-        unit = ItemFactory.create(
-            parent_location=sub_section.location,
+        self.other_sub_section = ItemFactory.create(
+            parent=self.section,
+            category='sequential',
         )
-        other_unit = ItemFactory.create(
-            parent_location=sub_section.location,
+        self.other_unit = ItemFactory.create(
+            parent=self.other_sub_section,
+            category='vertical',
         )
-        return section, sub_section, unit, other_unit
 
 
 class TestCourseStatusGET(CourseStatusAPITestCase, MobileAuthUserTestMixin, MobileEnrolledCourseAccessTestMixin):
@@ -115,13 +124,15 @@ class TestCourseStatusGET(CourseStatusAPITestCase, MobileAuthUserTestMixin, Mobi
     """
     def test_success(self):
         self.login_and_enroll()
-        (section, sub_section, unit, __) = self._setup_course_skeleton()
 
         response = self.api_response()
-        self.assertEqual(response.data["last_visited_module_id"], unicode(unit.location))
         self.assertEqual(
-            response.data["last_visited_module_path"],
-            [unicode(module.location) for module in [unit, sub_section, section, self.course]]
+            response.data["last_visited_module_id"],  # pylint: disable=no-member
+            unicode(self.sub_section.location)
+        )
+        self.assertEqual(
+            response.data["last_visited_module_path"],  # pylint: disable=no-member
+            [unicode(module.location) for module in [self.sub_section, self.section, self.course]]
         )
 
 
@@ -135,37 +146,45 @@ class TestCourseStatusPATCH(CourseStatusAPITestCase, MobileAuthUserTestMixin, Mo
 
     def test_success(self):
         self.login_and_enroll()
-        (__, __, __, other_unit) = self._setup_course_skeleton()
-
-        response = self.api_response(data={"last_visited_module_id": unicode(other_unit.location)})
-        self.assertEqual(response.data["last_visited_module_id"], unicode(other_unit.location))
+        response = self.api_response(data={"last_visited_module_id": unicode(self.other_unit.location)})
+        self.assertEqual(
+            response.data["last_visited_module_id"],  # pylint: disable=no-member
+            unicode(self.other_sub_section.location)
+        )
 
     def test_invalid_module(self):
         self.login_and_enroll()
         response = self.api_response(data={"last_visited_module_id": "abc"}, expected_response_code=400)
-        self.assertEqual(response.data, errors.ERROR_INVALID_MODULE_ID)
+        self.assertEqual(
+            response.data,  # pylint: disable=no-member
+            errors.ERROR_INVALID_MODULE_ID
+        )
 
     def test_nonexistent_module(self):
         self.login_and_enroll()
         non_existent_key = self.course.id.make_usage_key('video', 'non-existent')
         response = self.api_response(data={"last_visited_module_id": non_existent_key}, expected_response_code=400)
-        self.assertEqual(response.data, errors.ERROR_INVALID_MODULE_ID)
+        self.assertEqual(
+            response.data,  # pylint: disable=no-member
+            errors.ERROR_INVALID_MODULE_ID
+        )
 
     def test_no_timezone(self):
         self.login_and_enroll()
-        (__, __, __, other_unit) = self._setup_course_skeleton()
-
         past_date = datetime.datetime.now()
         response = self.api_response(
             data={
-                "last_visited_module_id": unicode(other_unit.location),
+                "last_visited_module_id": unicode(self.other_unit.location),
                 "modification_date": past_date.isoformat()  # pylint: disable=maybe-no-member
             },
             expected_response_code=400
         )
-        self.assertEqual(response.data, errors.ERROR_INVALID_MODIFICATION_DATE)
+        self.assertEqual(
+            response.data,  # pylint: disable=no-member
+            errors.ERROR_INVALID_MODIFICATION_DATE
+        )
 
-    def _date_sync(self, date, initial_unit, update_unit, expected_unit):
+    def _date_sync(self, date, initial_unit, update_unit, expected_subsection):
         """
         Helper for test cases that use a modification to decide whether
         to update the course status
@@ -182,36 +201,41 @@ class TestCourseStatusPATCH(CourseStatusAPITestCase, MobileAuthUserTestMixin, Mo
                 "modification_date": date.isoformat()
             }
         )
-        self.assertEqual(response.data["last_visited_module_id"], unicode(expected_unit.location))
+        self.assertEqual(
+            response.data["last_visited_module_id"],  # pylint: disable=no-member
+            unicode(expected_subsection.location)
+        )
 
     def test_old_date(self):
         self.login_and_enroll()
-        (__, __, unit, other_unit) = self._setup_course_skeleton()
         date = timezone.now() + datetime.timedelta(days=-100)
-        self._date_sync(date, unit, other_unit, unit)
+        self._date_sync(date, self.unit, self.other_unit, self.sub_section)
 
     def test_new_date(self):
         self.login_and_enroll()
-        (__, __, unit, other_unit) = self._setup_course_skeleton()
-
         date = timezone.now() + datetime.timedelta(days=100)
-        self._date_sync(date, unit, other_unit, other_unit)
+        self._date_sync(date, self.unit, self.other_unit, self.other_sub_section)
 
     def test_no_initial_date(self):
         self.login_and_enroll()
-        (__, __, _, other_unit) = self._setup_course_skeleton()
         response = self.api_response(
             data={
-                "last_visited_module_id": unicode(other_unit.location),
+                "last_visited_module_id": unicode(self.other_unit.location),
                 "modification_date": timezone.now().isoformat()
             }
         )
-        self.assertEqual(response.data["last_visited_module_id"], unicode(other_unit.location))
+        self.assertEqual(
+            response.data["last_visited_module_id"],  # pylint: disable=no-member
+            unicode(self.other_sub_section.location)
+        )
 
     def test_invalid_date(self):
         self.login_and_enroll()
         response = self.api_response(data={"modification_date": "abc"}, expected_response_code=400)
-        self.assertEqual(response.data, errors.ERROR_INVALID_MODIFICATION_DATE)
+        self.assertEqual(
+            response.data,  # pylint: disable=no-member
+            errors.ERROR_INVALID_MODIFICATION_DATE
+        )
 
 
 class TestCourseEnrollmentSerializer(MobileAPITestCase):

lms/djangoapps/mobile_api/users/views.py

@@ -107,15 +107,14 @@ class UserCourseStatus(views.APIView):
             course.id, request.user, course, depth=2)
 
         course_module = get_module_for_descriptor(request.user, request, course, field_data_cache, course.id)
-        current = course_module
 
-        path = []
-        child = current
-        while child:
-            path.append(child)
-            child = get_current_child(current)
-            if child:
-                current = child
+        path = [course_module]
+        chapter = get_current_child(course_module, min_depth=2)
+        if chapter is not None:
+            path.append(chapter)
+            section = get_current_child(chapter, min_depth=1)
+            if section is not None:
+                path.append(section)
 
         path.reverse()
         return path
@@ -160,7 +159,7 @@ class UserCourseStatus(views.APIView):
         save_positions_recursively_up(request.user, request, field_data_cache, module)
         return self._get_course_info(request, course)
 
-    @mobile_course_access()
+    @mobile_course_access(depth=2)
     def get(self, request, course, *args, **kwargs):  # pylint: disable=unused-argument
         """
         Get the ID of the module that the specified user last visited in the specified course.
@@ -168,7 +167,7 @@ class UserCourseStatus(views.APIView):
 
         return self._get_course_info(request, course)
 
-    @mobile_course_access()
+    @mobile_course_access(depth=2)
     def patch(self, request, course, *args, **kwargs):  # pylint: disable=unused-argument
         """
         Update the ID of the module that the specified user last visited in the specified course.

lms/djangoapps/mobile_api/utils.py

@@ -5,11 +5,13 @@ Common utility methods and decorators for Mobile APIs.
 
 import functools
 
-from opaque_keys.edx.keys import CourseKey
-from courseware.courses import get_course_with_access
 from rest_framework import permissions
 from rest_framework.authentication import OAuth2Authentication, SessionAuthentication
 
+from opaque_keys.edx.keys import CourseKey
+from xmodule.modulestore.django import modulestore
+from courseware.courses import get_course_with_access
+
 
 def mobile_course_access(depth=0, verify_enrolled=True):
     """
@@ -25,13 +27,14 @@ def mobile_course_access(depth=0, verify_enrolled=True):
             Raises 404 if access to course is disallowed.
             """
             course_id = CourseKey.from_string(kwargs.pop('course_id'))
-            course = get_course_with_access(
-                request.user,
-                'load_mobile' if verify_enrolled else 'load_mobile_no_enrollment_check',
-                course_id,
-                depth=depth
-            )
-            return func(self, request, course=course, *args, **kwargs)
+            with modulestore().bulk_operations(course_id):
+                course = get_course_with_access(
+                    request.user,
+                    'load_mobile' if verify_enrolled else 'load_mobile_no_enrollment_check',
+                    course_id,
+                    depth=depth
+                )
+                return func(self, request, course=course, *args, **kwargs)
         return _wrapper
     return _decorator
 

lms/djangoapps/mobile_api/video_outlines/serializers.py

@@ -3,6 +3,7 @@ Serializer for video outline
 """
 from rest_framework.reverse import reverse
 
+from xmodule.modulestore.mongo.base import BLOCK_TYPES_WITH_CHILDREN
 from courseware.access import has_access
 
 from edxval.api import (
@@ -14,10 +15,10 @@ class BlockOutline(object):
     """
     Serializes course videos, pulling data from VAL and the video modules.
     """
-    def __init__(self, course_id, start_block, categories_to_outliner, request):
+    def __init__(self, course_id, start_block, block_types, request):
         """Create a BlockOutline using `start_block` as a starting point."""
         self.start_block = start_block
-        self.categories_to_outliner = categories_to_outliner
+        self.block_types = block_types
         self.course_id = course_id
         self.request = request  # needed for making full URLS
         self.local_cache = {}
@@ -143,11 +144,11 @@ class BlockOutline(object):
                 # from the table-of-contents.
                 continue
 
-            if curr_block.category in self.categories_to_outliner:
+            if curr_block.location.block_type in self.block_types:
                 if not has_access(user, 'load', curr_block, course_key=self.course_id):
                     continue
 
-                summary_fn = self.categories_to_outliner[curr_block.category]
+                summary_fn = self.block_types[curr_block.category]
                 block_path = list(path(curr_block))
                 unit_url, section_url = find_urls(curr_block)
 
@@ -159,8 +160,17 @@ class BlockOutline(object):
                     "summary": summary_fn(self.course_id, curr_block, self.request, self.local_cache)
                 }
 
+            def parent_or_requested_block_type(usage_key):
+                """
+                Returns whether the usage_key's block_type is one of self.block_types or a parent type.
+                """
+                return (
+                    usage_key.block_type in self.block_types or
+                    usage_key.block_type in BLOCK_TYPES_WITH_CHILDREN
+                )
+
             if curr_block.has_children:
-                for block in reversed(curr_block.get_children()):
+                for block in reversed(curr_block.get_children(usage_key_filter=parent_or_requested_block_type)):
                     stack.append(block)
                     child_to_parent[block] = curr_block